Page MenuHomePhabricator

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

Authored by ashoat on Sat, Jun 8, 3:46 PM.
Referenced Files
F2030880: D12369.id41177.diff
Tue, Jun 18, 12:16 AM
F2030871: D12369.id41192.diff
Tue, Jun 18, 12:16 AM
F2026847: D12369.id41177.diff
Mon, Jun 17, 6:32 PM
F2026124: D12369.id41146.diff
Mon, Jun 17, 2:34 PM
Unknown Object (File)
Sun, Jun 16, 2:59 PM
Unknown Object (File)
Sun, Jun 16, 2:58 PM
Unknown Object (File)
Sun, Jun 16, 12:31 PM
Unknown Object (File)
Sat, Jun 15, 5:46 PM



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

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Sat, Jun 8, 4:05 PM
Harbormaster failed remote builds in B29559: Diff 41146!
ashoat requested review of this revision.Sat, Jun 8, 6:50 PM
ashoat added inline comments.
700–708 ↗(On Diff #41149)

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.
714 ↗(On Diff #41149)

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.Mon, Jun 10, 7:02 AM
700–708 ↗(On Diff #41149)

Agree with that - I think we can remove it

700–708 ↗(On Diff #41149)

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

714 ↗(On Diff #41149)

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.Mon, Jun 10, 2:03 PM
This revision was automatically updated to reflect the committed changes.