Network Cleanup: Isolate "persisted " queue from "main" queue #8312

marcaaron · 2022-03-25T00:52:48Z

Details

Another round of Network refactoring. For this one, I focused on making the "persisted requests queue" more distinct from the "main request queue" with renames and making files more modular.

Renamed ONYXKEYS.NETWORK_REQUEST_QUEUE to PERSISTED_REQUESTS because that's what we use it for and "network request queue" is ambiguous
Tried to clean up the callbacks and by moving them to a NetworkEvents file
Killed the concept of the queue being "paused" or "unpaused" and instead we use an "isAuthenticating" flag and refer to it the same everywhere. This always confused me because the queue is processed in a "loop" but we don't actually ever "pause" the loop we just return early when we processing while authenticating.
Make the "Network is ready" hack more obvious by changing the name to hasReadRequiredDataFromStorage()
Moved the methods related to persisted requests OUT of the Network.js file so it is more isolated and less likely to be confused as related to the main queue code (still want to improve this further)
Separate "request retry counting" logic into a RetryCounter class
Move the processRequest() method to it's own file
Added extra documentation to Network/index.js and extracted the logic that pulls requests out of the main queue and into the persist queue and the logic for retrying main queue requests when they fail
Renamed actions/NetworkRequestQueue to PersistedRequests and gave it a separate "retry counter" instance (h/t @roryabraham) as that got a little mixed up in this PR, but seems like a fine addition to have for now. Though I am a little unsure how this would look for users who get a fetch error while making a blocking request. Seems like by perpetually retrying it would take a long time for them to see something "stop loading".
Also updated tests

Fixed Issues

No issue but related to Offline First project

Tests

Automated tests should cover most of these changes as we are mainly refactoring

Run the app make sure things are working

Verify that no errors appear in the JS console

QA Steps

No specific QA as no behavior has changed, but keep a lookout for anything like loading spinners, offline stuff that was working not working, etc.

Verify that no errors appear in the JS console

… might be used

…orage thing

roryabraham

Overall super stoked about these changes and very impressed by how much you've been able to unpack our Network code to make it more approachable. Really, excellent work. Did have a few comments, questions, and suggestions.

src/libs/Network/NetworkEvents.js

src/libs/Network/NetworkStore.js

roryabraham · 2022-03-25T17:40:43Z

src/libs/Network/NetworkStore.js

-let networkReady = false;
+let hasReadRequiredData = false;
+let authenticating = false;
+let isOffline = false;


We can do all this later (as part of offline-first doc) if you prefer, but should we assume that we're offline unless we know otherwise?

Suggested change

let isOffline = false;

let isOffline = true;

Further, if we made that change, could we remove the self-proclaimed hack for checkRequiredData?

We assume that we're offline until we know otherwise

We won't know otherwise (and set isOffline = false unless Onyx has triggered the Onyx.connect callback for the NETWORK key

So we don't have to check if Onyx has read the authToken and credentials from storage before clearing the network lib to make request

Good thoughts. I don't have a clear answer about which would be better from an Offline First Doc perspective and would have to weigh some pros/cons.

I don't think we can remove that hack if we do this anyway as there's no guarantee we won't set isOffline to true before the credentials and authToken are ready. It's maybe adding some delay, but not the same as the guarantee we have now.

src/libs/Network/NetworkStore.js

roryabraham · 2022-03-25T18:39:31Z

src/libs/Network/processRequest.js

+ */
+export default function processRequest(request) {
+    const finalParameters = enhanceParameters(request.command, request.data);
+    const cancelRequestTimeoutTimer = NetworkEvents.startRequestTimeoutTimer();


Sorry, what happens if the request times out?

We "trigger a connection recheck", but I'll leave a comment

I kinda move this so we wouldn't have to explain what happens and just emit and event. But the event is hardcoded in :D

src/libs/Network/PersistedRequestsQueue.js

roryabraham · 2022-03-25T18:51:14Z

src/libs/Network/PersistedRequestsQueue.js

+    persistedRequestsQueueRunning = true;
+
+    // Ensure persistedRequests are read from storage before proceeding with the queue
+    const connectionId = Onyx.connect({


Sorry, it's probably just a me problem, but I'm confused how this is working and have a few questions:

Why connect and disconnect instead of just relying on Onyx.connect to keep the persisted requests up-to-date? This feels like a hack b/c we're simulating Onyx.get here, though I'm sure it's a necessary one for now.

What triggers the PersistedRequestsQueue to flush? The only usage I see is here, and that seems like it would only run on app init?

You're right. It's a hack that ensures we don't start processing the queue until the requests have been read from storage. This mostly only matters on app init so we could probably improve this further (as we are subscribing in PersistedRequests as well and could use that). We might be able to use the createOnReadyTask to workaround this for now (but I'm not gonna add that here).

What triggers the PersistedRequestsQueue to flush? The only usage I see is here, and that seems like it would only run on app init?

Main usage here and runs when we go from offline to online

App/src/libs/Network/PersistedRequestsQueue.js

Lines 90 to 91 in f4c1cb2

// Flush the queue when the connection resumes

NetworkEvents.onConnectivityResumed(flush);

marcaaron · 2022-03-25T22:28:34Z

Thanks for the review @roryabraham. Updated and hopefully answered all the questions you had!

roryabraham · 2022-03-28T17:13:44Z

Sounds like there are a few more improvements we could make, but we can do that in a follow-up PR(s). Thanks again for cleaning this up.

marcochavezf · 2022-03-28T17:32:00Z

src/libs/API.js

        return Network.post(originalCommand, originalParameters, originalType);
    }

    // Prevent any more requests from being processed while authentication happens
-    Network.pauseRequestQueue();
-    isAuthenticating = true;
+    NetworkStore.setIsAuthenticating(true);


NAB: I think we can use NetworkStore.isAuthenticating() and NetworkStore.setAuthenticating(value) for this boolean value, no? https://airbnb.io/javascript/#accessors--boolean-prefix

Maybe - though unsure if we follow that guidance strictly. I had it like that but changed it as I couldn't think of a way to do that without changing the variable name here...

App/src/libs/Network/NetworkStore.js

Line 11 in c39f2cf

let isAuthenticating = false;

I wouldn't see a problem renaming it to authenticating since it's only used as a private variable and the NetworkStore is a small file, but that's just my opinion :)

OSBotify · 2022-03-28T18:38:07Z

✋ 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:12Z

🚀 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:18Z

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

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

Julesssss · 2022-04-12T15:41:32Z

Hey, I'm slightly concerned that this PR may have introduced this exception (or at least, made the error more visible). This is the only recent PR I can see which touches the network code in the last couple of releases.

Specifically, I believe we're seeing one of these functions being called before being defined. Does that seem at all possible to you?

marcaaron · 2022-04-12T19:30:15Z

Specifically, I believe we're seeing one of these functions being called before being defined. Does that seem at all possible to you?

Yes, and it's probably this one ->

App/src/libs/NetworkConnection.js

Line 123 in 8419bd4

unsubscribeFromNetInfo();

Is it possible that this has existed before? I don't think we changed anything about when the recheck callback should run and this PR mostly was a re-organization of existing logic.

Julesssss · 2022-04-13T13:25:20Z

I'm slightly hesitant to move forward without better evidence, but yeah that does seem to be the case.

With the occurrences spiking on a single day and no specific code changes which directly modified the problematic area I think it's okay to continue with the release and continue to monitor the crash.

Julesssss · 2022-04-13T14:18:17Z

FYI I submitted a quick PR and I'm moving on with the release.

marcaaron · 2022-04-13T17:22:52Z

Thanks @Julesssss !

sobitneupane · 2023-02-07T12:40:47Z

This PR might have caused #14584 regression.

marcaaron · 2023-02-07T15:30:26Z

Thanks @sobitneupane. To clarify, it might have caused it or did cause it? If so, how? Can we figure it out?

sobitneupane · 2023-02-07T17:07:22Z

I thought introduction of following code in PersistedRequests.js might have caused the issue.

persistedRequests = lodashUnionWith(persistedRequests, requestsToPersist, _.isEqual);

But it was there even before and introduced by 5cd8b4a commit.

marcaaron · 2023-02-07T23:44:21Z

Got it, thanks for looking into that and clarifying.

I think these changes are old enough that there's nothing actionable and I checked off the steps here.

marcaaron added 6 commits March 24, 2022 10:16

Get rid of queue pause logics

204514d

Move callbacks to Events. Start separating logic more.

9193948

Refactor retry counter

5d61b2d

Fix canProcessImmediately confusion and add more context about why it…

0d5285c

… might be used

fix retryFailedRequest

4ce55d6

fix conflicts

d67fff7

marcaaron self-assigned this Mar 25, 2022

marcaaron added 2 commits March 24, 2022 15:03

use really ugly name to communicate the required data reading from st…

e16211f

…orage thing

update comment

f4c1cb2

marcaaron changed the title ~~[WIP] Network Cleanup Part 2~~ Network Cleanup Part 2 Mar 25, 2022

marcaaron changed the title ~~Network Cleanup Part 2~~ Network Cleanup: Isolate "persisted " queue from "main" queue Mar 25, 2022

marcaaron requested review from marcochavezf and roryabraham March 25, 2022 01:32

marcaaron marked this pull request as ready for review March 25, 2022 01:32

marcaaron requested a review from a team as a code owner March 25, 2022 01:32

melvin-bot bot requested review from stitesExpensify and removed request for a team March 25, 2022 01:32

marcaaron removed the request for review from stitesExpensify March 25, 2022 01:33

roryabraham requested changes Mar 25, 2022

View reviewed changes

marcaaron added 2 commits March 25, 2022 11:25

make event names more consistent

88279fe

Make requested changes

5792e80

marcaaron added 2 commits March 25, 2022 12:59

fix name

6d0576a

Merge remote-tracking branch 'origin' into marcaaron-networkCleanup2

c39f2cf

roryabraham approved these changes Mar 28, 2022

View reviewed changes

marcochavezf reviewed Mar 28, 2022

View reviewed changes

marcochavezf approved these changes Mar 28, 2022

View reviewed changes

marcaaron merged commit 52a7e05 into main Mar 28, 2022

marcaaron deleted the marcaaron-networkCleanup2 branch March 28, 2022 18:26

OSBotify mentioned this pull request Mar 30, 2022

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

Closed

47 tasks

Julesssss mentioned this pull request Apr 12, 2022

Deploy Checklist: New Expensify 2022-04-08 #8566

Closed

17 tasks

sobitneupane mentioned this pull request Feb 7, 2023

[HOLD for payment 2023-02-08] pinned chat become unpinned when user pinned chat in offline mode #14584

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network Cleanup: Isolate "persisted " queue from "main" queue #8312

Network Cleanup: Isolate "persisted " queue from "main" queue #8312

marcaaron commented Mar 25, 2022 •

edited

Loading

roryabraham left a comment •

edited

Loading

roryabraham Mar 25, 2022

marcaaron Mar 25, 2022

roryabraham Mar 25, 2022

marcaaron Mar 25, 2022

marcaaron Mar 25, 2022

roryabraham Mar 25, 2022

marcaaron Mar 25, 2022

marcaaron commented Mar 25, 2022

roryabraham commented Mar 28, 2022

marcochavezf Mar 28, 2022

marcaaron Mar 28, 2022

marcochavezf Mar 28, 2022

OSBotify commented Mar 28, 2022

OSBotify commented Mar 30, 2022

OSBotify commented Apr 5, 2022

Julesssss commented Apr 12, 2022 •

edited

Loading

marcaaron commented Apr 12, 2022

Julesssss commented Apr 13, 2022

Julesssss commented Apr 13, 2022

marcaaron commented Apr 13, 2022

sobitneupane commented Feb 7, 2023

marcaaron commented Feb 7, 2023

sobitneupane commented Feb 7, 2023

marcaaron commented Feb 7, 2023

	// Flush the queue when the connection resumes
	NetworkEvents.onConnectivityResumed(flush);

Network Cleanup: Isolate "persisted " queue from "main" queue #8312

Network Cleanup: Isolate "persisted " queue from "main" queue #8312

Conversation

marcaaron commented Mar 25, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

roryabraham left a comment • edited Loading

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 Mar 25, 2022

roryabraham commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Mar 28, 2022

OSBotify commented Mar 30, 2022

OSBotify commented Apr 5, 2022

Julesssss commented Apr 12, 2022 • edited Loading

marcaaron commented Apr 12, 2022

Julesssss commented Apr 13, 2022

Julesssss commented Apr 13, 2022

marcaaron commented Apr 13, 2022

sobitneupane commented Feb 7, 2023

marcaaron commented Feb 7, 2023

sobitneupane commented Feb 7, 2023

marcaaron commented Feb 7, 2023

marcaaron commented Mar 25, 2022 •

edited

Loading

roryabraham left a comment •

edited

Loading

Julesssss commented Apr 12, 2022 •

edited

Loading