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): ensure only one RPC connect request can go out at once #5322

Merged
merged 2 commits into from
Apr 7, 2020

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Mar 30, 2020

overview

This PR patches a couple weird usability issues with the RPC client/server interaction. The RPC server is a broadcast server; it will issue messages to all connected WebSockets, and it does not care if the same client establishes multiple WebSockets.

On the app end, if the client creates multiple connections to the RPC server, weird things can and do happen in the app state because it's not built to handle this. In lieu of messing around with the RPC server, which is generally too dangerous to touch without good reason / care, this PR adds multiple layers of protection on the client side to prevent multiple RPC connections from being established simultaneously:

  1. At the UI level, the two connection buttons will be disabled if there is a connection request in flight
  2. At the state level, if a connect action still manages to happen even though the buttons are disabled, the state reducer will throw away any new connect action if there is already a connect in progress
  3. At the RPC client worker level, if a connect action manages to make it to the worker even though the buttons are disabled, the client worker will also throw away the action if a connect is already in progress

In addition to the buttons getting disabled, this PR adds a spinner state to the main "connect" button when the request is in-flight. While the toggle is fully disabled when a request is in flight, I chose not to gray it out. My reasoning behind this choice was:

  • Every single toggle in the list would gray out for a second when the request is in flight, which might look funny
  • Clicking on the toggle automatically brings you to the robot page, where you will see the main connect button replaced with a spinner

Closes #5241, closes #5307

longer term plans

In the long term, the RPC server will be replaced. Clicking the "connect" button will, instead of doing this whole RPC handshake + serialization + deserialization dance, make a single HTTP request to lock the robot control. Further seeding of state once control has been granted will happen in the background over multiple HTTP requests, with the UI gracefully updating as those requests come back

changelog

  • fix(app): ensure only one RPC connect request can go out at once

review requests

  • Try to get the app into a weird connection state! Take a look at 5241 and 5307 for some examples

risk assessment

Medium

  • Code that touches RPC functionality is risky because the whole system is risky
  • All changes were covered with unit tests
  • Existing UI that had no unit tests were given tests for good measure

@mcous mcous added app Affects the `app` project ready for review fix PR fixes a bug rpc This involves Opentrons' deprecated RPC system. labels Mar 30, 2020
@mcous mcous requested review from SyntaxColoring and a team March 30, 2020 21:45
@mcous mcous requested a review from a team as a code owner March 30, 2020 21:45
@mcous mcous requested review from amitlissack and sanni-t and removed request for a team March 30, 2020 21:45
@codecov
Copy link

codecov bot commented Mar 30, 2020

Codecov Report

Merging #5322 into edge will increase coverage by 16.92%.
The diff coverage is 98.46%.

Impacted file tree graph

@@             Coverage Diff             @@
##             edge    #5322       +/-   ##
===========================================
+ Coverage   47.27%   64.20%   +16.92%     
===========================================
  Files         885     1077      +192     
  Lines       13331    32130    +18799     
===========================================
+ Hits         6302    20628    +14326     
- Misses       7029    11502     +4473     
Impacted Files Coverage Δ
app/src/robot/selectors.js 80.50% <92.85%> (+0.12%) ⬆️
app/src/components/ConnectPanel/RobotItem.js 100.00% <100.00%> (+100.00%) ⬆️
app/src/components/ConnectPanel/RobotListItem.js 100.00% <100.00%> (+100.00%) ⬆️
app/src/components/RobotSettings/StatusCard.js 100.00% <100.00%> (+100.00%) ⬆️
app/src/robot/api-client/client.js 90.75% <100.00%> (+0.03%) ⬆️
app/src/robot/constants.js 100.00% <100.00%> (ø)
app/src/robot/reducer/connection.js 90.90% <100.00%> (ø)
...er/src/components/modals/EditModulesModal/index.js 90.90% <0.00%> (-6.71%) ⬇️
app/src/components/RobotSettings/ControlsCard.js 84.37% <0.00%> (-4.52%) ⬇️
app/src/analytics/make-event.js 70.64% <0.00%> (-1.49%) ⬇️
... and 208 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c602cd6...169787d. Read the comment docs.

Copy link
Contributor

@amitlissack amitlissack left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dozercodes dozercodes left a comment

Choose a reason for hiding this comment

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

LGTM

@mcous mcous merged commit 9465cef into edge Apr 7, 2020
@mcous mcous deleted the app_single-rpc-connection branch May 6, 2020 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project fix PR fixes a bug rpc This involves Opentrons' deprecated RPC system.
Projects
None yet
3 participants