Retry requests if storage (auth + credentials) is not ready by marcochavezf · Pull Request #6183 · Expensify/App

marcochavezf · 2021-11-02T23:48:25Z

Details

There's an issue on Android where the user is logged out randomly. We're not 100% sure about the root cause, but one possibility is the existence of a race condition between the async storage and the networks requests; where the auth token and credentials are not read yet from persistent storage (Onyx) while a network request is made.

I was able to reproduce this scenario by adding a setTimeout in the Onyx.connect callback (more details in this comment):

Screen.Recording.2021-10-12.at.18.32.38.mov

There's a long discussion in the issue, where different approaches are discussed (both big and small changes). But we agreed on tackling the issue atm without changing the architecture of the Network Queue. This solution is comprised of the following points:

Add a unit test (StoreNetworkTest.js) that simulates the scenario where auth token + credentials are not ready.
Add the flag isReady and its setter to Network.js to process requests only if auth token + credentials are already read from storage, otherwise re-queue the requests to retry later (as long as the request don't have the property doNotRetry = true). Also, log a message to the server when this scenario happens.
Stop logging out in enhanceParameters (which is addDefaultValuesToParameters in API.js), since we're already handling that auth token + credentials are read first and the http request should logout the user if the auth token is inexistent or expired (server should return a 407 jsonCode if we are online). Also, the API.js has changed a lot since the logout (redirectToSignIn()) was added and probably is no longer needed because that's already handled in handleExpiredAuthToken.

Also a few extra comments:

In order to use the Log in Network I disabled the eslint circular dependency warning (hopefully we can fix this later with a better dependency or another design).
Fixed a few minor jsdocs types.
Added explicit Jest globals to get rid of errors in vscode and have better IntelliSense.
I updated some unit tests to verify the retry is not happening (which is an ideal scenario where auth token + credentials are already read when a network request happens).
Also added Onyx.init and Onyx.clear in some tests, because for some reason the Onyx.connect callback was not triggered and those tests were failing.

Fixed Issues

$ #5619

Tests / QA Steps

In Chrome devtools go to Application -> Local Storage and:

Mess up the authToken (session) and make sure you are reauthenticated.
Mess up the credentials and authToken and make sure you are logged out.

N/A

Tested On

Screenshots

N/A

- add explicit globals to improve intellisense in vscode

move isReady to canMakeRequest

(let the http request logout if the auth token is expired or inexistent)

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

enhanceParameters and its related canMakeRequest call

move StoreNetworkTest to NetworkTest

and update unit test

nickmurray47

The changes lgtm afaict - looks like there's one linting issue and also wondering if we can add manual tests for regressions to make sure we aren't breaking anything.

marcochavezf · 2021-11-04T22:30:39Z

Since we are setting the Network to "ready" when the session and credentials are available should we set it back to "not ready" when we sign out? If the Network was still in the "ready" state and we tried to sign in I'm curious if we would run into any issues if e.g. their local variables were not yet set? 🤔

Hmm I don't think so, because the Network.setIsReady(true) should be only called when the app is booting up to avoid the race condition between storage and network. Once session and credentials (existent or non-existent) are read from storage the Network should be ready until the app is closed by the OS. I think we should only set the network to "not ready" if the storage fails for some reason while the app is still running on a handset (not sure if that's something the OS handles making the app crash, but in any case should be a rare scenario).

marcaaron · 2021-11-04T22:44:15Z

Hmm I don't think so, because the Network.setIsReady(true) should be only called when the app is booting up to avoid the race condition between storage and network.

Yeah I was thinking that when you sign out the storage is cleared so the values for session and credentials look like this:

But since we "cache-first" there's no way for an Onyx.set() or Onyx.merge() to cause a "slow" Onyx.connect() on sign in.

https://github.com/Expensify/react-native-onyx/blob/224e2ba03f2365726b8c1d0785e750c54a85332b/lib/Onyx.js#L530-L537

change unit test comments to GIVE-WHEN-THEN format

marcochavezf · 2021-11-04T23:17:21Z

Hmm I don't think so, because the Network.setIsReady(true) should be only called when the app is booting up to avoid the race condition between storage and network.

Yeah I was thinking that when you sign out the storage is cleared so the values for session and credentials look like this:

But since we "cache-first" there's no way for an Onyx.set() or Onyx.merge() to cause a "slow" Onyx.connect() on sign in.

https://github.com/Expensify/react-native-onyx/blob/224e2ba03f2365726b8c1d0785e750c54a85332b/lib/Onyx.js#L530-L537

Ah yeah but even if we don't read the values from the cache after signing out, I don't think we should have a problem; the storage and the network are ready and the network loop should handle any bad state.

marcaaron

LGTM 👍

It would be nice to see if the test can be improved further, but maybe not crucial to do right now. I think what we've come up works OK for now.

marcaaron · 2021-11-04T23:34:56Z

 * @param {Object} personalDetails
 * @param {String} searchValue
- * @param {Array<String>} betas
+ * @param {Array<String>=} betas


Forgot this one

marcaaron · 2021-11-04T23:38:59Z

Maybe merging main will fix the e2e test or we can restart it.

marcochavezf · 2021-11-05T19:06:54Z

It would be nice to see if the test can be improved further, but maybe not crucial to do right now. I think what we've come up works OK for now.

Yeah, hopefully, we can improve later this test (which I think we'll require a few changes in API.js to have better control of the Onyx.connect callbacks). Also in the coverage report (running npm run test -- --coverage) I noticed we cover 75.58% of Network.js code in the unit tests:

And given the importance of Network.js in the app, should we add more unit tests to cover if possible 100% in this file? 🤔

Maybe merging main will fix the e2e test or we can restart it.

I added another commit with more comments in the unit test and now the e2e test passed 😄

marcaaron · 2021-11-05T19:17:31Z

Left a follow up comment about the JS doc, but I think this is good to merge.

This reverts commit ec2ee08.

nickmurray47 · 2021-11-05T21:13:31Z

Not sure if tests failing had anything to do with the recent change but restarted them to see if they'll pass again

nickmurray47

Tests pass now! Feel free to merge and @marcochavezf do you mind adding these QA steps to the PR from this comment:

Mess up the authToken and make sure you are reauthenticated
Mess up the credentials and authToken and make sure you are logged out

Conflicts: src/libs/Network.js

OSBotify · 2021-11-08T16:53:14Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-11-15T21:21:39Z

🚀 Deployed to staging by @marcaaron in version: 1.1.14-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

isagoico · 2021-11-16T14:24:30Z

@marcaaron @marcochavezf Hello! I'm not sure if were testing this correctly. What I did was:

Navigate to Application in dev tools
In local storage search for session
In the session info I changed the Authtoken and the email. I just added a bunch of random characters to mess it up

After that, I was not logged out and it looked like I was logged in as the user I randomly wrote on the email

marcochavezf · 2021-11-17T14:52:39Z

@marcaaron @marcochavezf Hello! I'm not sure if were testing this correctly. What I did was:

Navigate to Application in dev tools

In local storage search for session

In the session info I changed the Authtoken and the email. I just added a bunch of random characters to mess it up

After that, I was not logged out and it looked like I was logged in as the user I randomly wrote on the email

Ah from the app side I think this is fine because we're only verifying that authToken (from session object) and the credentials object (which is another object in the local storage) are valid or existent to keep the user logged in (which are retrieved from the server when the user enters their correct email and password). And the email from the session object is only used for the UI and not used to re-login the user.

OSBotify · 2021-11-22T20:10:48Z

🚀 Deployed to production by @AndrewGable in version: 1.1.15-15 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

marcochavezf added 6 commits November 2, 2021 11:09

retry a request if auth+crendentials are not ready

bb59f93

disable circular dep between Log-Api-Network

bb69c23

- add Onyx initialization

8552b0a

- add explicit globals to improve intellisense in vscode

add unit test to check if auth+cred are not ready

79bcfb6

add documentation to isReady

2bbddeb

move isReady to canMakeRequest

stop logging the user out in enhanceParameters

9c2ff14

(let the http request logout if the auth token is expired or inexistent)

marcochavezf requested a review from a team as a code owner November 2, 2021 23:48

MelvinBot requested review from nickmurray47 and removed request for a team November 2, 2021 23:48

marcochavezf changed the title ~~Retry requests if storage (auth + credentials) is not ready~~ [HOLD] Retry requests if storage (auth + credentials) is not ready Nov 2, 2021

marcochavezf changed the title ~~[HOLD] Retry requests if storage (auth + credentials) is not ready~~ Retry requests if storage (auth + credentials) is not ready Nov 3, 2021

marcochavezf self-assigned this Nov 3, 2021

marcaaron self-requested a review November 3, 2021 01:18

marcaaron requested changes Nov 3, 2021

View reviewed changes

marcochavezf and others added 14 commits November 3, 2021 11:16

Update tests/unit/StoreNetworkTest.js

56b0bb8

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

Update src/libs/Network.js

ab15fa6

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

Update tests/actions/SessionTest.js

acfc39a

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

Update tests/actions/ReportTest.js

d0f9acc

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

add suggestions to API.js and StoreNetworkTest.js

480b81c

remove code to pause and clear queue in

b4b694e

enhanceParameters and its related canMakeRequest call

add delay to AsyncStorage.getItem()

78da9e4

move StoreNetworkTest to NetworkTest

remove comment in canMakeRequest

3713f70

and update unit test

check if the network request is retried in test

9ac5627

restore mock async-storage to its original verison

a67d885

add Onyx mock to add delay in connect callbacks

60df933

fix jsdoc param for GetAccountStatus

0b2f305

fix test to retry request if Network is not ready

1e0a163

add missing optional param in jsdoc

ec2ee08

nickmurray47 reviewed Nov 4, 2021

View reviewed changes

add waitForPromisesToResolve in test

62ef020

change runOnlyPendingTimers to advanceTimersByTime

ea99e0b

marcaaron reviewed Nov 4, 2021

View reviewed changes

Comment thread tests/unit/NetworkTest.js Outdated

add PROCESS_REQUEST_DELAY_MS to Network

63d35a8

change unit test comments to GIVE-WHEN-THEN format

marcaaron previously approved these changes Nov 4, 2021

View reviewed changes

update comment in test that retrires network req

b8a580b

marcochavezf dismissed marcaaron’s stale review via b8a580b November 5, 2021 16:15

marcaaron previously approved these changes Nov 5, 2021

View reviewed changes

Revert "add missing optional param in jsdoc"

1f23139

This reverts commit ec2ee08.

marcochavezf dismissed marcaaron’s stale review via 1f23139 November 5, 2021 19:51

nickmurray47 previously approved these changes Nov 7, 2021

View reviewed changes

marcochavezf added 2 commits November 8, 2021 10:07

Merge remote-tracking branch 'refs/remotes/origin/main'

61f2df0

Conflicts: src/libs/Network.js

remove duplicated lodashGet

c6fb00f

marcochavezf dismissed nickmurray47’s stale review via c6fb00f November 8, 2021 15:09

disable prefer-onyx-connect-in-libs in Onyx mock

7f22cab

marcaaron approved these changes Nov 8, 2021

View reviewed changes

marcaaron merged commit ccdf729 into main Nov 8, 2021

marcaaron deleted the marco-checkIfOnyxIsReady3 branch November 8, 2021 16:52

OSBotify mentioned this pull request Nov 15, 2021

Deploy Checklist: New Expensify 2021-11-15 #6305

Closed

70 tasks

Conversation

marcochavezf commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests / QA Steps

Tested On

Screenshots

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickmurray47 left a comment

Choose a reason for hiding this comment

Uh oh!

marcochavezf commented Nov 4, 2021

Uh oh!

marcaaron commented Nov 4, 2021

Uh oh!

Uh oh!

marcochavezf commented Nov 4, 2021

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

marcaaron Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Nov 4, 2021

Uh oh!

marcochavezf commented Nov 5, 2021

Uh oh!

marcaaron commented Nov 5, 2021

Uh oh!

nickmurray47 commented Nov 5, 2021

Uh oh!

nickmurray47 left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Nov 8, 2021

Uh oh!

OSBotify commented Nov 15, 2021

Uh oh!

isagoico commented Nov 16, 2021

Uh oh!

marcochavezf commented Nov 17, 2021

Uh oh!

OSBotify commented Nov 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marcochavezf commented Nov 2, 2021 •

edited

Loading