Block `SequentialQueue` on reauthentication. Reorganize Network / Reauthentication code. #8742

marcaaron · 2022-04-21T21:30:26Z

Details

Makes the SequentialQueue work better when we are reauthenticating.

Before: When a request in the SQ needed reauthentication we would call Authenticate and then send the original request into the non-sequential queue (where it would be processed after the SQ empties). That is bad because requests happen out of order.

Request 1 (Fail)
Authenticate
Request 2
Request 3
Request 1

After: The reauthentication logic has been reorganized and calls to Authenticate will block the queue

Request 1 (Fail)
Authenticate
Request 1 
Request 2
Request 3

Fixed Issues

https://github.com/Expensify/Expensify/issues/207534

Tests (Build staging version of app)

Go offline via Chrome Dev Tools Network tab
Leave 10 comments on a report
Invalidate the authToken via Preferences
Go back online
Verify by inspecting the Network requests tab in Chrome Dev Tools that each request in the queue is made sequentially and that the first request requiring reauthentication will wait for Authenticate to complete before moving onto the next request in the queue.

Verify that no errors appear in the JS console

PR Review Checklist

PR Reviewer Checklist

QA Steps

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

…est()

…cationInSequentialQueue

marcaaron · 2022-04-26T01:39:29Z

@tgolen This PR got a little out of hand - but I think in a good way.

So many things that were bugging me about the Network code feel pretty nice now with the middleware idea applied.

Here's the basic idea...

Network.post() passes requests to the queues
The queues call Request.process() which calls HttpUtils.xhr() with middlewares applied
Each middleware returns a promise and passes the response to the next middleware
The order is important and we have:
- Logging - Logs the request details (this cleaned up all circular dependencies with Log)
- Recheck connection - Not sure how much we need the stuff in this file, but figured I'd leave it for now.
- Reauthentication - Handles reauthentication
- Retry - Handles request retry logic

One thing that is still bugging me about this (but will be fixed shortly) is that I've got a isFromSequentialQueue argument getting passed to the middleware. This is necessary because I haven't yet forced all network requests that can be persisted directly into the sequential queue. Once we do that we can just look at the request object and see if it's "persistable", but for now, isFromSequentialQueue is the only way to tell if it was called from the sequential queue or not.

tgolen

Overall I think this looks really clean and organized. I like how the middleware pattern keeps logic very self-contained and purposeful.

src/libs/Middleware/Reauthentication.js

tgolen · 2022-04-26T15:54:30Z

src/libs/Middleware/Reauthentication.js

+                }
+
+                // We are already authenticating
+                if (NetworkStore.getIsAuthenticating()) {


What do you think about moving this to Authenticate.isAuthenticating()?

I think the only problem with that is Authentication has a dependency on Network (it calls post()) and we also use this in the main queue logic to see if we should make a request (which would create a circular dependency and is the main reason why I created the NetworkStore idea to keep track of these things for us).

One idea I am having is to create an AuthenticationUtils.

I've tried a couple different things and didn't land on how to fix the dependency cycle (I need more practice fixing those puzzles 😄 ), but it maybe is reasonable to have authenticate and reauthenticate in Authentication and put the credentials and isAuthenticating stuff in an AuthenticationUtils

Ah, circular dependencies can be a huge pain like that. I think I'd rather leave them on NetworkStore rather than having a new AuthenticationUtils.

It's at least better that Network doesn't have any specialized knowledge now, but it is a little hard trying to reason about the difference between Network and NetworkStore.

tgolen · 2022-04-26T15:55:16Z

src/libs/Middleware/Reauthentication.js

+                        // This can happen if we are already in the process of authenticating via the main queue and then
+                        // the sequential queue gets flushed (e.g. because we went offline while we were authenticating, queued some requests
+                        // and then came back online which triggers the sequential queue to flush).
+                        // I think the solution is to maybe only ever have one call to Authenticate happening at any time no matter who calls it.


I think I agree with this solution but I'm not sure what that might imply.

Yeah, I haven't quite worked it out, but I think it'd be something like...

We only ever have one call to Authenticate happening at a time and we have a reference to that promise in the middleware

If a request is made and that promise hasn't resolved yet we wait until it does by hooking into it and using it's response

I will caveat that I also haven't observed the case I'm describing happen, but did write a failing test that seems to imply it could happen like this:

Request made while online returns 407 which triggers a call to Authenticate to get queued, but not run

We go offline the same time the response is received but before we actually call Authenticate (not sure if this is possible)

We make some requests while offline

We get "stuck" isAuthenticating (again, not sure if it's possible)

The Sequential Queue runs and the first request gets a 407

We can't continue because we are "already authenticating"

This is why I had the instinct to throw an error - which would reveal if it's possible or not.

Actually, the situation I am describing can be prevented by just checking to see if we are offline when handling a 407 and throwing an error if we are. That seems like a simpler solution so gonna try that.

Ah, cool. In the end, if we still really want to log for this case, then I think we can just leave a comment that says it's there for a bit to ensure that nothing ever happens (because if it did happen, we would never know it, and it would lead to bugs that would be very hard to debug) and maybe we can go in later and remove the error.

tgolen · 2022-04-26T15:56:27Z

src/libs/Middleware/Reauthentication.js

+                            throw new Error('Unable to reauthenticate sequential queue request because we failed to reauthenticate');
+                        }
+
+                        Network.replayRequest(request);


If this request is replayed, wouldn't it just fail to authenticate again?

A comment could help here (or maybe there's some way to improve this).

We have to dig into the extra logic in reauthenticate(), but it is likely to succeed as it most likely failed because of a networking issue. If we fail because the credentials are bad then we just get logged out.

App/src/libs/Authentication.js

Lines 128 to 136 in dcaea12

// When a fetch() fails and the "API is offline" error is thrown we won't log the user out. Most likely they

// have a spotty connection and will need to try to reauthenticate when they come back online. We will

// re-throw this error so it can be handled by callers of reauthenticate().

if (error.message === CONST.ERROR.API_OFFLINE) {

throw error;

}

// If we experience something other than a network error then redirect the user to sign in

redirectToSignIn(error.message);

marcaaron · 2022-04-26T20:35:05Z

Ok ready for review now.
Gonna add another reviewer here since the changes are pretty extensive (though a lot of this is just reorganizing).
I am open to splitting this stuff up into several PRs, but would have to think about how to do it.

tgolen

I love the unit tests! Just some suggestions on names that might help make their purpose more clear.

src/libs/API.js

tgolen · 2022-04-26T21:27:47Z

src/libs/API.js

-function Authenticate(parameters) {
-    const commandName = 'Authenticate';
+// Recheck - Sets a timeout timer for a request that will "recheck" if we are connected to the internet if time runs out. Also triggers the connection recheck when we encounter any error.
+Request.use(Middleware.Recheck);


Suggestion: rename to Middleware.RecheckConnection

This just gives it a little more context. When I first saw this, I wasn't sure what "recheck" meant at all.

yep, great suggestion

src/libs/Authentication.js

src/libs/Middleware/Recheck.js

src/libs/Request.js

tgolen · 2022-04-26T21:39:00Z

src/libs/API.js

-    // All other jsonCode besides 407 are treated as a successful response and must be handled in the .then() of the API method
-    queuedRequest.resolve(response);
-});
+// Setup API middlewares. Each request made will pass through a series of middleware functions that will get called in sequence (each one passing the result of the previous to the next).


It might be helpful to add some comments about the order they are used in and why that order matters.

Added some more context below.

marcaaron · 2022-04-27T17:29:05Z

Updated thanks for the notes @tgolen ! 🙇

tylerkaraszewski

I won't pretend to be an expert in this code, but I did read all of it. Only had a couple comments.

src/libs/Authentication.js

tylerkaraszewski · 2022-04-27T17:52:22Z

src/libs/Middleware/Reauthentication.js

+                        throw new Error('Missing credentials required for authentication');
+                    }
+
+                    MainQueue.replay(request);


I don't get get what this does. If we failed for lack of credentials, we try again? Will this automatically get credentials somewhere?

Yeah, this is a really great find and I think points to a deeper problem.

The comment suggests that "Credentials haven't been initialized". So we are putting this request back into the queue because we are "waiting for the credentials". Which begs these questions:

Why we are waiting for them at all ?

Why we are not waiting for them in the sequential queue ?

Gonna think about this for a second. My gut says no requests should happen at all until we can guarantee the credentials either exist or don't exist. Then we can just throw here and the user should probably also get signed out.

Looked into this a bit. The changes required here are a bit involved but I think worth doing...

We have some logic here:

App/src/libs/Network/MainQueue.js

Lines 23 to 27 in 58a139a

// We must attempt to read authToken and credentials from storage before allowing any requests to happen so that any requests that

// require authToken or trigger reauthentication will succeed.

if (!NetworkStore.hasReadRequiredDataFromStorage()) {

return false;

}

and it needs a better design since both the "main queue" and "sequential queue" should only make requests when we are "ready" to do so.

Going to create a follow up issue for this as it's slightly tricky to untangle and I don't think it needs to be a blocker.

TL;DR - no, credentials are not automatically regenerated and this code appears related to a race condition. the queue we have for these requests now isn't "waiting for the credentials" so it's not clear if this is a problem or not, but should be cleaned up.

Created an issue here: https://github.com/Expensify/Expensify/issues/208303

thienlnam

Also not familiar with network code but the changes look great! Just a couple questions

src/libs/Authentication.js

marcaaron · 2022-04-29T16:07:43Z

Gonna merge since we have a couple of approvals.

@tylerkaraszewski let me know if you have any thoughts about my response here.

OSBotify · 2022-04-29T16:16:41Z

✋ 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 · 2022-05-09T20:04:06Z

🚀 Cherry-picked to staging by @sketchydroide in version: 1.1.57-8 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2022-05-13T16:13:02Z

🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀

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

Santhosh-Sellavel · 2023-06-03T03:01:34Z

src/libs/Middleware/Reauthentication.js

+            if (NetworkStore.isOffline()) {
+                // If we are offline and somehow handling this response we do not want to reauthenticate
+                throw new Error('Unable to reauthenticate because we are offline');
+            }


This PR causes a regression here more details on the PR

Block SequentialQueue on reauthentication

1163fb3

marcaaron self-assigned this Apr 21, 2022

marcaaron added 12 commits April 21, 2022 15:40

Fix tests

26c7219

reorganize some things

65361f1

Move setIsAuthenticating call to reauthenticate

a72a8e7

Add more docs to explain the reauthenticate method inside processRequ…

6d8fd83

…est()

improve comments

91869dc

Merge remote-tracking branch 'origin' into marcaaron-handleReauthenti…

23fbcfd

…cationInSequentialQueue

Move Authenticate/reauthenticate logic to Authentication lib

e483ded

Authentication middleware

578f9f8

Use middleware for process request

55b507a

Add Retry middleware. Kill NetworkEvents

04b6ef4

Fix test

d34f7dd

Remove unneeded error constants

6802bf3

marcaaron added 6 commits April 25, 2022 15:40

Remove fake jsonCode

d8e90aa

Remove makeSequentialRequest

11cf2f8

Minimize some changes related to Network.post()

838c9b6

minimize more API changes

41ef3b3

Add comments / fix up middleware functions

fa37610

Write failing test for Authenticate edge case

dcaea12

tgolen reviewed Apr 26, 2022

View reviewed changes

marcaaron added 6 commits April 26, 2022 07:03

Add comments + fix failing test

2a1b9a8

add comment

3977a87

Create MainQueue and AuthenticationUtils

dde135d

Fix conflicts

cdff656

Add test for request middleware feature

0478110

Kill AuthenticationUtils

617478e

marcaaron changed the title ~~[WIP] Block SequentialQueue on reauthentication~~ Block SequentialQueue on reauthentication Apr 26, 2022

marcaaron marked this pull request as ready for review April 26, 2022 20:20

marcaaron requested a review from a team as a code owner April 26, 2022 20:20

melvin-bot bot requested review from tylerkaraszewski and removed request for a team April 26, 2022 20:20

marcaaron requested a review from a team April 26, 2022 20:33

melvin-bot bot requested review from thienlnam and removed request for a team April 26, 2022 20:33

marcaaron changed the title ~~Block SequentialQueue on reauthentication~~ Block SequentialQueue on reauthentication. Reorganize Network / Reauthentication code. Apr 26, 2022

tgolen requested changes Apr 26, 2022

View reviewed changes

marcaaron added 2 commits April 26, 2022 13:27

Make requested naming changes

386254d

Fix comment

58a139a

tylerkaraszewski reviewed Apr 27, 2022

View reviewed changes

Fix Log.hmmm

f1c8b4f

thienlnam previously approved these changes Apr 27, 2022

View reviewed changes

src/libs/Authentication.js Show resolved Hide resolved

src/libs/Authentication.js Show resolved Hide resolved

marcaaron dismissed thienlnam’s stale review via f1c8b4f April 27, 2022 19:28

thienlnam approved these changes Apr 27, 2022

View reviewed changes

marcaaron requested a review from tgolen April 28, 2022 23:52

tgolen approved these changes Apr 29, 2022

View reviewed changes

marcaaron merged commit 69e3e28 into main Apr 29, 2022

marcaaron deleted the marcaaron-handleReauthenticationInSequentialQueue branch April 29, 2022 16:07

OSBotify mentioned this pull request May 10, 2022

Deploy Checklist: New Expensify 2022-04-25 #8777

Closed

jjcoffee mentioned this pull request Apr 26, 2023

[HOLD for payment 2023-05-23] [$2000] System displays a technical error message when handling an error (Auth CreateReportAction returned an error) #16637

Closed

6 tasks

Santhosh-Sellavel reviewed Jun 3, 2023

View reviewed changes

Santhosh-Sellavel mentioned this pull request Jun 3, 2023

[HOLD for payment 6-7-2023] Dev: Name is not centered in Share Code page #19241

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block `SequentialQueue` on reauthentication. Reorganize Network / Reauthentication code. #8742

Block `SequentialQueue` on reauthentication. Reorganize Network / Reauthentication code. #8742

marcaaron commented Apr 21, 2022 •

edited

Loading

marcaaron commented Apr 26, 2022

tgolen left a comment

tgolen Apr 26, 2022

marcaaron Apr 26, 2022

marcaaron Apr 26, 2022

tgolen Apr 26, 2022

tgolen Apr 26, 2022

marcaaron Apr 26, 2022

marcaaron Apr 26, 2022

tgolen Apr 26, 2022

tgolen Apr 26, 2022

marcaaron Apr 26, 2022

marcaaron commented Apr 26, 2022

tgolen left a comment

tgolen Apr 26, 2022

marcaaron Apr 26, 2022

tgolen Apr 26, 2022

marcaaron Apr 26, 2022 •

edited

Loading

marcaaron commented Apr 27, 2022

tylerkaraszewski left a comment

tylerkaraszewski Apr 27, 2022

marcaaron Apr 27, 2022

marcaaron Apr 27, 2022

marcaaron Apr 27, 2022

marcaaron Apr 27, 2022

marcaaron Apr 27, 2022

tylerkaraszewski Apr 29, 2022

thienlnam left a comment

marcaaron commented Apr 29, 2022

OSBotify commented Apr 29, 2022

OSBotify commented May 9, 2022

OSBotify commented May 13, 2022

Santhosh-Sellavel Jun 3, 2023

	// When a fetch() fails and the "API is offline" error is thrown we won't log the user out. Most likely they
	// have a spotty connection and will need to try to reauthenticate when they come back online. We will
	// re-throw this error so it can be handled by callers of reauthenticate().
	if (error.message === CONST.ERROR.API_OFFLINE) {
	throw error;
	}

	// If we experience something other than a network error then redirect the user to sign in
	redirectToSignIn(error.message);

	// We must attempt to read authToken and credentials from storage before allowing any requests to happen so that any requests that
	// require authToken or trigger reauthentication will succeed.
	if (!NetworkStore.hasReadRequiredDataFromStorage()) {
	return false;
	}

Block SequentialQueue on reauthentication. Reorganize Network / Reauthentication code. #8742

Block SequentialQueue on reauthentication. Reorganize Network / Reauthentication code. #8742

Conversation

marcaaron commented Apr 21, 2022 • edited Loading

Details

Fixed Issues

Tests (Build staging version of app)

PR Review Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron commented Apr 26, 2022

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Apr 26, 2022

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

marcaaron commented Apr 27, 2022

tylerkaraszewski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thienlnam left a comment

Choose a reason for hiding this comment

marcaaron commented Apr 29, 2022

OSBotify commented Apr 29, 2022

OSBotify commented May 9, 2022

OSBotify commented May 13, 2022

Choose a reason for hiding this comment

Block `SequentialQueue` on reauthentication. Reorganize Network / Reauthentication code. #8742

Block `SequentialQueue` on reauthentication. Reorganize Network / Reauthentication code. #8742

marcaaron commented Apr 21, 2022 •

edited

Loading

marcaaron Apr 26, 2022 •

edited

Loading