-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(app-shell, app-shell-odd): Fix intermittently dropped notifications #14477
fix(app-shell, app-shell-odd): Fix intermittently dropped notifications #14477
Conversation
Because subscription logic is directly tied to the component lifecycle, it is possible for a component to trigger an unsubscribe event on dismount while a new component mounts and triggers a subscribe event. For the connection store and MQTT to reflect correct topic subscriptions, do not unsubscribe and close connections before newly mounted components have had time to update the connection store.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.2.0 #14477 +/- ##
=======================================================
+ Coverage 67.70% 67.81% +0.11%
=======================================================
Files 1628 2519 +891
Lines 54905 72200 +17295
Branches 4149 9360 +5211
=======================================================
+ Hits 37171 48961 +11790
- Misses 17043 21012 +3969
- Partials 691 2227 +1536
Flags with carried forward coverage won't be shown. Click here to find out more.
|
See https://opentrons.atlassian.net/browse/RAUT-959 for discussion on the better way to do this. Changes involving base connectivity won't make it into the 7.2 release but will be prioritized for future MQTT work. Once that's done, moving unsubscribe logic to the server side (per @sfoster1 's advice) will be do-able. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested, looks good
…ns (#14477) Closes RQA-2339, RQA-2321, RQA-2319, RAUT-962, RQA-2346 * fix(app-shell, app-shell-odd): fix intermittently dropped notifications Because subscription logic is directly tied to the component lifecycle, it is possible for a component to trigger an unsubscribe event on dismount while a new component mounts and triggers a subscribe event. For the connection store and MQTT to reflect correct topic subscriptions, do not unsubscribe and close connections before newly mounted components have had time to update the connection store.
Closes RQA-2339, RQA-2321, RQA-2319, RAUT-962
Overview
Our subscribe/unsubscribe logic is directly tied to the component lifecycle. If a component dismounts and unsubscribes to a given topic while a new component mounts and subscribes to the same topic, a race condition occurs in which the shell layer often thinks the subscribing component is subscribed when it is in fact not. This race condition occurs when switching pages that contain components on each page interested in the same topic.
The most straightforward fix is to set a small timeout any time an unsubscribe request goes through to accommodate any additional rendering that might occur. After the timeout, then check to see if the connection manager really wants to unsubscribe.
I think this solution makes the most sense, but I'm open to alternatives.
Some things I considered:
Test Plan
Changelog
Risk assessment
low