fix(app, app-shell, app-shell-odd): block initial HTTP request until successful MQTT subscription #15094
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…cription Currently, the desktop app/ODD attempt to address the "missed update problem" in the following way: while we subscribe to a topic, we simultaneously GET whatever equivalent HTTP resource we just subscribed to. However, there's definitely a world (albeit a very small one) in which we receive the HTTP response, a server update occurs, the server publishes, and then we successfully subscribe to a topic. In this world, we've missed the update event. Solve for this by simply blocking the initial HTTP GET until we subscribe. While the subscribe handshake could theoretically take a maximum of 2 seconds (at which point we forcefully timeout the subscribe action and fallback to polling), in practice it's more like 250ms. We already handle failed connections and don't go through this handshake if we can't connect to the client to begin with, so the "wait 2 second until sub failure" scenario shouldn't realistically happen. This bug was a product of initially using retained messages during prototyping, however we removed retained messaging by MQTT launch.
mjhuff
changed the title
sfoster1
requested changes
May 6, 2024
There was a problem hiding this comment.
I'm a little hesitant to introduce more latency here. What if instead we send the synthetic refresh again after we establish the connection? We have an extra refresh, but we don't extend the so-called time to first light, and we're still way better off than when we were just polling.
Yeah that makes a lot of sense. |
sfoster1
approved these changes
May 7, 2024
Carlos-fernandez
pushed a commit
that referenced
this pull request
May 20, 2024
…successful MQTT subscription (#15094) Closes EXEC-429 Currently, the desktop app/ODD attempt to address the "missed update problem" in the following way: while we subscribe to a topic, we simultaneously GET whatever equivalent HTTP resource we just subscribed to. However, there's definitely a world (albeit a very small one) in which we receive the HTTP response, a server update occurs, the server publishes, and then we successfully subscribe to a topic. In this world, we've missed the update event. Solve for this by simply refetching right after the client subscribe ACKs. We still keep the initial fetch on mount to keep latency low.
Carlos-fernandez
pushed a commit
that referenced
this pull request
Jun 3, 2024
…successful MQTT subscription (#15094) Closes EXEC-429 Currently, the desktop app/ODD attempt to address the "missed update problem" in the following way: while we subscribe to a topic, we simultaneously GET whatever equivalent HTTP resource we just subscribed to. However, there's definitely a world (albeit a very small one) in which we receive the HTTP response, a server update occurs, the server publishes, and then we successfully subscribe to a topic. In this world, we've missed the update event. Solve for this by simply refetching right after the client subscribe ACKs. We still keep the initial fetch on mount to keep latency low.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes EXEC-429
Overview
Currently, the desktop app/ODD attempt to address the "missed update problem" in the following way: while we subscribe to a topic, we simultaneously GET whatever equivalent HTTP resource we just subscribed to. However, there's definitely a world (albeit a very small one) in which we receive the HTTP response, a server update occurs, the server publishes, and then we successfully subscribe to a topic. In this world, we've missed the update event.
Solve for this by simply blocking the initial HTTP GET until we subscribe. While the subscribe handshake could theoretically take a maximum of 2 seconds (at which point we forcefully timeout the subscribe action and fallback to polling), in practice it's more like <~250ms (wall-clock request to response). We already handle failed connections and don't go through this handshake if we can't connect to the client to begin with, so the "wait 2 second until sub failure" scenario shouldn't realistically happen.
Edge vs. 7.3.0?
After discussing with @SyntaxColoring, we believe it's best not to include the fix in 7.3.0, since there's always the possibility we cause a worse problem than we solve (although I feel good that this isn't the case). The worst case scenario is the client very very rarely doesn't receive an update notification as expected.
In practice, this problem has yet to be reported by QA or identified by a dev, so I think it's better to hold off. I'm willing to be convinced otherwise, though.
Test Plan
Changelog
Risk assessment
Medium(ish). See subsection in overview.