Page MenuHomePhabricator

[lib][native][web] Recover from socket crash loop using fetchPendingUpdates
ClosedPublic

Authored by ashoat on Jun 8 2024, 3:46 PM.
Tags
None
Referenced Files
F3491895: D12369.diff
Wed, Dec 18, 9:08 PM
Unknown Object (File)
Sun, Dec 15, 3:05 AM
Unknown Object (File)
Sun, Dec 15, 3:05 AM
Unknown Object (File)
Sun, Dec 15, 3:05 AM
Unknown Object (File)
Sun, Dec 15, 3:05 AM
Unknown Object (File)
Sun, Dec 15, 3:04 AM
Unknown Object (File)
Sun, Dec 15, 3:03 AM
Unknown Object (File)
Wed, Nov 27, 2:20 AM
Subscribers

Details

Summary

This addresses ENG-8089.

Depends on D12368

Test Plan

In combination with later diffs, I tested as follows:

  1. I created a socket crash loop on a physical iOS device using @inka's create-many-threads-to-trigger-crash-loop.js script (see ENG-8090
  2. I confirmed that the socket was unable to connect prior to my diff stack
  3. I confirmed that after applying my diff stack, the SUCCESS action was dispatched, and the socket was able to connect afterwards

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 8 2024, 4:05 PM
Harbormaster failed remote builds in B29559: Diff 41146!
ashoat requested review of this revision.Jun 8 2024, 6:50 PM
ashoat added inline comments.
lib/socket/socket.react.js
700–708

Thinking about this more, I wonder if we should just get rid of this older mechanism. It has the potential of logging the user out if recovery is not possible (web, or ETH user on native). Removing it would also let us deprecate showSocketCrashLoopAlert, which only exists to warn the user about the potential logout.

I think the new mechanism should allow us to heal from any socket crash loop, including one that is the result of a policy acknowledgment after login. cc @kamil

tomek added inline comments.
lib/socket/socket.react.js
714

Should we somehow remember that this operation is in progress? I worry that it might be possible to start this action multiple times.

This revision is now accepted and ready to land.Jun 10 2024, 7:02 AM
lib/socket/socket.react.js
700–708

Agree with that - I think we can remove it

lib/socket/socket.react.js
700–708

Great – I'll remove it in a follow-up

714

Good call. Will add something for this

Make sure we only ever call fetchPendingUpdates once at a time

This revision was landed with ongoing or failed builds.Jun 10 2024, 2:03 PM
This revision was automatically updated to reflect the committed changes.