Cleanup Network.js code. Fix persisted request handling. by marcaaron · Pull Request #8306 · Expensify/App

marcaaron · 2022-03-24T19:55:45Z

Details

This PR does some cleanup of the Network code to help get things ready for Sync Write Queue changes that we will be adding soon. I will be following up with more PRs to modularize and organize the code, but for this PR the changes are:

Replace HTTP_STATUS_CODE const with JSON_CODE (since a status code and a jsonCode are NOT the same and this could easily confuse people)
There were very strange logics in the "retryable persistent requests" area of the code.
- We were retrying all non 200 jsonCode requests by throwing an error. This doesn't really make sense because if you get a 404 or something then we should not retry.
- If we got a 407 we do not reauthenticate. That doesn't really make sense because we might have an expired session while trying to run a persisted request and should reauthenticate and retry the original request in that case not continue to retry it. Basically, anything that returns a jsonCode can just have the response handled by onResponse
- We were also retrying ANY request that triggers the .catch(). I checked server logs and we have mostly thrown Failed to fetch and a jsonCode 407. I updated this logic so that only "Failed to fetch" errors will get "retried"
- For every other kind of possible error I added an ENSURE_BUGBOT log line so we can see what these errors actually are and then decide how to handle them.
Our wrapper around fetch() is not tracking bad responses or doing anything about them. I added a throw to HttpUtils.xhr() (which will trigger the ENSURE_BUGBOT so we can see what these things are and handle them).
Cleaned up logic for enhanceParameters and moved it to a separate file instead of using the "callback" trick.
Moved some Onyx.connect() into a "store" file so that we can check for the "network ready" and update the local authToken from API.js.

Fixed Issues

No issue, but related to the Offline First doc preparation

Tests

Automated tests were added
Do QA steps
Verify that no errors appear in the JS console

QA Steps

Test behavior of offline queue

Leave a comment on a report while offline
Close tab
Come back online
Verify comment has been created and is no longer grey

Verify that no errors appear in the JS console

Fix tests. Move enhanceParameters to its own method add docs fix bad comment clean up some logic that does not quite make sense Add more logging Only treat "Failed to fetch" errors as retryable for now. Curious to see what other kinds of errors we get. Throw any fetch errors when the response is not ok Make tests pass. Stop retrying non 200 jsonCode. fix up tests Add a few more tests Fix tests

marcaaron · 2022-03-24T19:59:28Z

cc @roryabraham too if you can give this a look

src/libs/API.js

marcochavezf · 2022-03-24T22:33:45Z

src/libs/Network/index.js

+                getLogger().info('Persisted optimistic request returned a valid jsonCode. Not retrying.');
            }
-
+            onResponse(request, response);


NAB: wouldn't be better to move onResponse to processRequest function?

I'm gonna look into this in my next clean up PR. Basically there are two different processRequest happening here. One for regular requests run in the "main" queue and another for the persisted requests. Yes, I think there's probably a way to have just one handler for both. But I don't want to touch that yet.

marcochavezf · 2022-03-24T22:44:07Z

src/libs/Network/index.js

+            // If we are catching a known network error like "Failed to fetch" allow this request to be retried if we have retries left
+            if (error.message === CONST.ERROR.FAILED_TO_FETCH) {
+                const retryCount = NetworkRequestQueue.incrementRetries(request);
+                getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message});
+                if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) {
+                    getLogger().info('Request failed too many times removing from storage', false, {retryCount, command: request.command, error: error.message});
+                    NetworkRequestQueue.removeRetryableRequest(request);
+                }
+            } else if (error.name === CONST.ERROR.REQUEST_CANCELLED) {
+                getLogger().info('Persisted request was cancelled. Not retrying.');
+                onError(request);
+                NetworkRequestQueue.removeRetryableRequest(request);
+            } else {
+                getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error while retrying persisted request. Not retrying.`, {
+                    command: request.command,
+                    error: error.message,
+                });


NAB: I think we can also move this logic to processRequest, no?

Same answer as before.

marcochavezf · 2022-03-24T22:46:15Z

src/libs/Network/index.js

+                if (error.name === CONST.ERROR.REQUEST_CANCELLED) {
+                    onError(queuedRequest, error);
+                    return;
+                }


NAB: Also I wonder if we can / should handle this condition in processRequest 🤔

Yes, but next PR please. And you can give me some ideas on how to do it if you have them 😄

marcochavezf

LGTM I just left a few comments that I think can be useful for the next refactoring PRs

src/libs/Network/NetworkStore.js

roryabraham · 2022-03-24T22:12:00Z

src/libs/Network/NetworkStore.js

+/**
+ * @param {String} newAuthToken
+ */
+function setAuthToken(newAuthToken) {


Why expose this function to set the authToken directly, instead updating SESSION.authToken in Onyx and relying on the Onyx.connect callback to update the local authToken?

Good eye. It's not quite a hack, but close. Setting values in Onyx always takes at least one tick of the event loop and updates subscribers late. If you look at where this is used it might make more sense.

App/src/libs/API.js

Lines 236 to 237 in 9c6c2a9

authToken: parameters.authToken,

shouldRetry: false,

Inside reauthenticate() we are setting the "local" authToken since anything called after reauthenticate() will need immediate access to the authToken - otherwise it will use the old one again and get a 407 again.

I'll leave a comment here. Basically, Onyx being Onyx though.

Thanks, makes sense.

Setting values in Onyx always takes at least one tick of the event loop and updates subscribers late ... Basically, Onyx being Onyx though

Is there a way we could fix this? Maybe we could create an issue in the Onyx repo?

Yeah, I think we even tried once, but it broke things in unexpected ways and there is a high likelihood that it may break stuff again. Anyone using Onyx.merge() is expecting to have those values available on the next tick (or later). Our current tests are pretty dependent on that "feature" as well so it's gonna be interesting if we ever try and do anything about it.

Hmmm seems to me like things might be a bit easier to reason about if one could safely assume than any value written in Onyx will be updated immediately, but I'll take your word for it.

They would 1000% be easier. And we could still do it. But whoever does it will need to check everything carefully and probably remove a lot of waitForPromisesToResolve() calls from our tests 😂

Created an issue: Expensify/react-native-onyx#123

It's not very detail-rich, but will hopefully explore more when I have some spare cycles.

roryabraham · 2022-03-24T22:13:30Z

src/libs/Network/enhanceParameters.js

+ */
+function isAuthTokenRequired(command) {
+    return !_.contains([
+        'Log',


NAB alphabetize this list

src/libs/Network/enhanceParameters.js

src/libs/Network/index.js

roryabraham · 2022-03-24T22:26:52Z

src/libs/Network/index.js

-                    const retryCount = NetworkRequestQueue.incrementRetries(queuedRequest);
-                    getLogger().info('A retrieable request failed', false, {
-                        retryCount,
+                // Retry and request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario.


Suggested change

// Retry and request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario.

// Retry any request that returns a "Failed to fetch" error. Very common if a user is offline.

Yes to the typo. Gonna leave the last part. There are other scenarios and they are unlikely.

Added some of those scenarios.

roryabraham · 2022-03-24T22:32:49Z

tests/unit/NetworkTest.js

+const originalXHR = HttpUtils.xhr;
+
+beforeEach(() => {
+    HttpUtils.xhr = originalXHR;


NAB/nothing new, but isn't it weird that our HttpUtils reference xhr when really we use fetch?

Seems like HttpUtils.request might be a more fitting name 🤷

src/libs/API.js

roryabraham · 2022-03-24T23:06:43Z

No blockers for me, just had some questions / minor suggestions. All of this is good cleanup, thanks for helping to tame this difficult code @marcaaron 🙇

marcaaron · 2022-03-25T00:07:17Z

Thanks for the reviews ! Updated and tried to add some better comments about the confusing things.

OSBotify · 2022-03-25T00:56:15Z

✋ 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-03-30T20:49:27Z

🚀 Deployed to staging by @marcaaron in version: 1.1.47-0 🚀

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

OSBotify · 2022-04-05T17:31:33Z

🚀 Deployed to production by @roryabraham in version: 1.1.49-1 🚀

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

marcaaron self-assigned this Mar 24, 2022

marcaaron requested a review from a team as a code owner March 24, 2022 19:55

marcaaron requested review from marcochavezf and roryabraham March 24, 2022 19:55

remove unused import

1bd7554

marcochavezf reviewed Mar 24, 2022

View reviewed changes

src/libs/API.js Show resolved Hide resolved

marcochavezf reviewed Mar 24, 2022

View reviewed changes

marcochavezf previously approved these changes Mar 24, 2022

View reviewed changes

roryabraham reviewed Mar 24, 2022

View reviewed changes

Add clarifying comments

cbe2b6e

marcaaron dismissed marcochavezf’s stale review via cbe2b6e March 24, 2022 23:58

Fix comment

fb3904c

roryabraham approved these changes Mar 25, 2022

View reviewed changes

marcaaron merged commit ee5d18e into main Mar 25, 2022

marcaaron deleted the marcaaron-networkCleanup branch March 25, 2022 00:46

roryabraham mentioned this pull request Mar 25, 2022

Make Onyx values update instantly Expensify/react-native-onyx#123

Closed

OSBotify mentioned this pull request Mar 30, 2022

Deploy Checklist: New Expensify 2022-03-30 #8378

Closed

47 tasks

	// Retry and request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario.
	// Retry any request that returns a "Failed to fetch" error. Very common if a user is offline.

Conversation

marcaaron commented Mar 24, 2022

Details

Fixed Issues

Tests

QA Steps

Uh oh!

marcaaron commented Mar 24, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcochavezf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roryabraham Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roryabraham Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roryabraham commented Mar 24, 2022

Uh oh!

marcaaron commented Mar 25, 2022

Uh oh!

OSBotify commented Mar 25, 2022

Uh oh!

OSBotify commented Mar 30, 2022

Uh oh!

OSBotify commented Apr 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

roryabraham Mar 25, 2022 •

edited

Loading

marcaaron Mar 25, 2022 •

edited

Loading

roryabraham Mar 25, 2022 •

edited

Loading