Skip to content
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

Merged

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Feb 12, 2024

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:

  1. We could in theory never unsubscribe to topics, but that means the MQTT broker holds more subscriptions in memory until the robot is power cycled (or the broker is restarted).
  2. Alternatively, we could somehow not handle subscriptions within components. This sounds great in theory, but our routes are not always entirely static (ex, we subscribe to runs/abc123, which we may not need 5 minutes from now).

Test Plan

  • Push this branch to the ODD.
  • On the desktop app, navigate to the DeviceDetails page.
  • Launch a protocol. The desktop app should display the run status in the upper right corner.
  • On the ODD, pause/play a protocol repeatedly. The desktop app run status should update.
  • The ODD buttons should update as expected.
  • Cancelling a protocol on the ODD should cause the "cancelling" modal to eventually unrender.

Changelog

  • Fixed an issue in which the app and ODD did not correctly reflect the current protocol run status.

Risk assessment

low

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.
@mjhuff mjhuff requested review from sfoster1 and a team February 12, 2024 20:19
@mjhuff mjhuff requested review from a team as code owners February 12, 2024 20:19
@mjhuff mjhuff requested review from koji and removed request for a team February 12, 2024 20:19
Copy link

codecov bot commented Feb 12, 2024

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (180d9ca) 67.70% compared to head (f204d01) 67.81%.
Report is 5 commits behind head on chore_release-7.2.0.

Additional details and impacted files

Impacted file tree graph

@@                   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     
Flag Coverage Δ
app 64.73% <0.00%> (+30.86%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
app-shell-odd/src/notify.ts 0.00% <0.00%> (ø)
app-shell/src/notify.ts 0.00% <0.00%> (ø)

... and 892 files with indirect coverage changes

@mjhuff mjhuff marked this pull request as draft February 12, 2024 20:58
@mjhuff mjhuff marked this pull request as ready for review February 12, 2024 21:59
@mjhuff
Copy link
Contributor Author

mjhuff commented Feb 12, 2024

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.

Copy link
Collaborator

@ncdiehl11 ncdiehl11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested, looks good

@mjhuff mjhuff merged commit 9badd57 into chore_release-7.2.0 Feb 13, 2024
21 of 22 checks passed
@mjhuff mjhuff deleted the app-shell_fix-intermittently-dropped-notifications branch February 13, 2024 21:41
TamarZanzouri pushed a commit that referenced this pull request Feb 16, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants