Network: Persisted Requests Queue - clear persisted requests after the response #6556

kidroca · 2021-12-01T10:53:20Z

Details

Additional functionality was added to Network.js and NetworkRequestQueue.js to better handle persisted requests

clear requests from persistent storage only after they are completed
retry persisted request

Added unit tests covering persisted request handling

Fixed Issues

$ #5987

Tests

Launch the app
Go offline
Write a few messages
Close the app
Be online
Open the app
Observe the messages sent while being offline are correctly sent to the backend

QA Steps

Follow the steps from the bug report

Make sure you are offline.
Send some messages to any user.
Refresh the page on the web. Close & open the app on mobile.
Now you can still see the messages.
Now enable internet
Shortly (a second after resuming the connection) after refresh or close the app
The messages should be posted after the refresh or after you open the app again

note: the order of the messages might be incorrect, this PR is not addressing the order

This comment has additional details on steps and timing: #5987 (comment)

Tested On

Screenshots

Web

New.Expensify.-.Google.Chrome.2021-12-01.12-51-05.mp4

Mobile Web

Android.Emulator.-.Pixel_2_API_29_5554.2021-12-01.13-11-03.mp4

Desktop

Screen.Recording.2021-12-01.at.13.22.10.mov

iOS

Screen.Recording.2021-12-01.at.14.45.00.mov

Android

Android.Emulator.-.Pixel_2_API_29_5554.2021-12-01.08-49-22.mp4

One less SESSION connection

Move more Onyx related handling as we're already dealing with saving and clearing here

Use _.isEqual to skip adding requests that we might already have persisted and the same when removing to remove only the matching request

kidroca

Added some highlights on the changes

kidroca · 2021-12-01T10:56:11Z

src/libs/API.js

+    // Unless email is already set include current user's email in every request and the server logs
+    finalParameters.email = lodashGet(parameters, 'email', currentUserEmail);


This was originally part of the network "forEach" here:

Network.js

// If we haven't passed an email in the request data, set it to the current user's email if (email && _.isEmpty(requestEmail)) { requestData.email = email; } const finalParameters = _.isFunction(enhanceParameters) ? enhanceParameters(queuedRequest.command, requestData) : requestData;

Here (addDefaultParameters) seems to be a better place for this logic

kidroca · 2021-12-01T11:01:02Z

src/libs/Network.js

+    // Do a recursive call in case the queue is not empty after processing the current batch
+    return Promise.all(tasks)
+        .then(processPersistedRequestsQueue);
+}


We can have remaining requests when

the request failed for some reason and is not removed from the queue - as seen in the catch block

we became offline again, which would again lead to the catch block above. In that case the queue would still stop and will be retriggered once we're back online, more requests can be added to it in the meantime

Not sure if we should capture that in a comment or something

kidroca · 2021-12-01T11:03:06Z

src/libs/Network.js

@@ -328,7 +316,6 @@ function clearRequestQueue() {
 export {
    post,
    pauseRequestQueue,
-    PROCESS_REQUEST_DELAY_MS,


This is now exported from CONST.NETWORK.PROCESS_REQUEST_DELAY_MS and usages updated

kidroca · 2021-12-01T11:05:24Z

src/libs/actions/NetworkRequestQueue.js

+function incrementRetries(request) {
+    const current = retryMap.get(request) || 0;
+    const next = current + 1;
+    retryMap.set(request, next);
+
+    return next;
+}


Retry counts are not persisted to disk - no need to
If a request is retried 5 times then the app quit, when the app is launched again the request will have a fresh count of up to 10 attempts, and not only the remaining 5

Might be an edge case but I think it would be best to have a retry limit in case some corrupt data was persisted and it cannot be sent no matter how many times we try.
That's why we keeping a count per request here and we forfeit a request after the count grows past a certain limit (CONST.NETWORK.MAX_PERSISTED_REQUEST_RETRIES)

johnmlee101 · 2021-12-09T15:50:09Z

@roryabraham do you mind co-reviewing this with me?

src/CONST.js

roryabraham · 2021-12-14T15:12:27Z

src/libs/Network.js

 let isQueuePaused = false;
+let persistedRequestsQueueRunning = false;


It's interesting that this value is sort of the inverse of isQueuePaused. I think we should try to keep these two things consistent, so maybe one of the following two options:

let isQueuePaused = false; let isOfflineQueuePaused = true;

or

let isMainQueueRunning = true; let isOfflineQueueRunning = false;

The name comes from this usagage and readability reasons
This method is starting something rather than making it not paused

function flushPersistedRequestsQueue() { if (persistedRequestsQueueRunning) { return; } persistedRequestsQueueRunning = true;

vs

function flushPersistedRequestsQueue() { if (!persistedRequestsQueuePaused) { return; } persistedRequestsQueuePaused = false;

running is the "special case" for this queue, while paused is the special case for the regular queue

I'm reluctant to refactor isQueuePaused to isMainQueueRunning it would change usages and might affect their intent as well, also it kind a sound like we're online too

src/libs/Network.js

roryabraham · 2021-12-14T16:07:06Z

I had only surface-level comments on this implementation, but will bring higher-level discussion to the linked issue.

kidroca · 2021-12-17T18:10:28Z

This is ready for review
I've made some small changes to address Rory's concerns.

mallenexpensify · 2022-01-06T20:19:22Z

@johnmlee101 @roryabraham can you review this PR? @kidroca 's been waiting three weeks

kidroca · 2022-01-14T20:15:46Z

Sorry, this accidentally got closed: #4908 (comment)

kidroca · 2022-01-26T19:48:10Z

@johnmlee101 @roryabraham
Can you post an update
What do I need to do to move this forward?

kidroca · 2022-01-26T19:49:18Z

Oh, I see there are conflicts
I'll be able to merge and test again tomorrow

kidroca · 2022-01-28T20:57:22Z

@johnmlee101 @roryabraham @mallenexpensify
Before I merge main, fix the conflicts and retest everything can someone at least confirm that's the only thing left to do and there are no requested changed

johnmlee101 · 2022-01-28T21:44:56Z

I think the changes right now are good, and I'll be ready to approve once the conflicts are resolved.

roryabraham · 2022-01-28T22:24:39Z

Yeah, I looked this over again and I don't think I have any further comments.

kidroca · 2022-01-31T16:29:05Z

I'll submit a merge and fix conflicts shortly

# Conflicts: # tests/unit/NetworkTest.js

kidroca · 2022-01-31T19:02:49Z

Synced with main and retested - everything working as intended

OSBotify · 2022-01-31T20:17:17Z

✋ 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-02-01T18:47:16Z

🚀 Deployed to staging by @roryabraham in version: 1.1.33-4 🚀

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

OSBotify · 2022-02-02T18:51:40Z

🚀 Deployed to production by @sketchydroide in version: 1.1.34-0 🚀

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

sobitneupane · 2023-02-07T17:13:47Z

src/libs/actions/NetworkRequestQueue.js

 }

 function saveRetryableRequests(retryableRequests) {
-    Onyx.merge(ONYXKEYS.NETWORK_REQUEST_QUEUE, retryableRequests);
+    persistedRequests = lodashUnionWith(persistedRequests, retryableRequests, _.isEqual);


This change has introduced this bug. (Title: pinned chat become unpinned when user pinned chat in offline mode)

If a user takes an action that is identical(_.isEqual) to older action then it will be removed.

To fix the issue we have replace above code by:

persistedRequests = persistedRequests.concat(requestsToPersist);

The PR that fixes the issue: #14608

The idea is to prevent the user from accumulate duplicate requests while offline
If you perform the same action 10x while offline, we skip posting 10 Network request but just one
This is an optimization that helps users in low network conditions to not waste bandwidth

From the information shared here it seems that pinning and unpinning the chat uses the same command / request, I would suggest to make them distinct actions so that _.isEqual can differentiate them correctly

We shouldn't have done this. Context https://expensify.slack.com/archives/C01GTK53T8Q/p1685822789307339

kidroca added 10 commits December 1, 2021 11:10

Network: extract processRequest logic

cd23f5f

Network: move email setting to parameter enhancer

bbf477a

One less SESSION connection

Network: define the new persisted request retry flow

136423b

Network: remove old persisted requests cleanup logic

ef1c058

NetworkRequestQueue: move persistedRequests here

4a5f7d0

Move more Onyx related handling as we're already dealing with saving and clearing here

NetworkRequestQueue: move the removeHandler from Network

6a61ea1

NetworkRequestQueue: add request filter/compare logic

5cd8b4a

Use _.isEqual to skip adding requests that we might already have persisted and the same when removing to remove only the matching request

Network: add tests for the persisted queue

729d4e6

Network: start queues only after init

66c8c0b

CONST: add NETWORK.SUCCESS_CODE

0746818

kidroca commented Dec 1, 2021

View reviewed changes

kidroca marked this pull request as ready for review December 1, 2021 12:48

kidroca requested a review from a team as a code owner December 1, 2021 12:48

MelvinBot removed the request for review from a team December 1, 2021 12:48

botify requested review from johnmlee101 and a team December 1, 2021 12:48

MelvinBot removed the request for review from a team December 1, 2021 12:48

kidroca mentioned this pull request Dec 6, 2021

Messages sent while offline disappear after connection is regained if the user closes/reopen app - Reported by: @Santhosh-Sellavel #5987

Closed

johnmlee101 requested a review from roryabraham December 9, 2021 15:50

This was referenced Dec 13, 2021

[Discussion] Add the ability to handle images in Onyx #3867

Closed

Onyx File handling - preparation #4908

Merged

roryabraham reviewed Dec 14, 2021

View reviewed changes

kidroca mentioned this pull request Dec 14, 2021

File handling POC Expensify/react-native-onyx#102

Merged

kidroca added 2 commits December 14, 2021 22:01

Merge branch 'main' into kidroca/persisted-request-queue

62b4208

Network: address PR requested changes

0ffab79

kidroca requested a review from roryabraham December 14, 2021 23:52

kidroca mentioned this pull request Jan 11, 2022

Network: Retry requests that failed in flight #6567

Merged

5 tasks

thienlnam closed this in #4908 Jan 14, 2022

thienlnam reopened this Jan 14, 2022

Merge branch 'main' into kidroca/persisted-request-queue

32adfa0

# Conflicts: # tests/unit/NetworkTest.js

roryabraham approved these changes Jan 31, 2022

View reviewed changes

roryabraham merged commit c2e34c1 into Expensify:main Jan 31, 2022

OSBotify mentioned this pull request Feb 1, 2022

Deploy Checklist: New Expensify 2022-02-01 #7504

Closed

46 tasks

kidroca deleted the kidroca/persisted-request-queue branch February 14, 2022 19:45

thienlnam mentioned this pull request Feb 28, 2022

Attachment upload requests are only persisted while offline #7935

Closed

marcaaron mentioned this pull request Mar 25, 2022

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

Merged

2 tasks

sobitneupane reviewed Feb 7, 2023

View reviewed changes

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: Persisted Requests Queue - clear persisted requests after the response #6556

Network: Persisted Requests Queue - clear persisted requests after the response #6556

kidroca commented Dec 1, 2021 •

edited

Loading

kidroca left a comment

kidroca Dec 1, 2021

kidroca Dec 1, 2021

kidroca Dec 1, 2021

kidroca Dec 1, 2021 •

edited

Loading

johnmlee101 commented Dec 9, 2021

roryabraham Dec 14, 2021

kidroca Dec 14, 2021

roryabraham commented Dec 14, 2021

kidroca commented Dec 17, 2021

mallenexpensify commented Jan 6, 2022

kidroca commented Jan 14, 2022

kidroca commented Jan 26, 2022

kidroca commented Jan 26, 2022

kidroca commented Jan 28, 2022

johnmlee101 commented Jan 28, 2022

roryabraham commented Jan 28, 2022

kidroca commented Jan 31, 2022

kidroca commented Jan 31, 2022

OSBotify commented Jan 31, 2022

OSBotify commented Feb 1, 2022

OSBotify commented Feb 2, 2022

sobitneupane Feb 7, 2023 •

edited

Loading

kidroca Feb 7, 2023

iwiznia Jun 5, 2023

		// Unless email is already set include current user's email in every request and the server logs
		finalParameters.email = lodashGet(parameters, 'email', currentUserEmail);

		let isQueuePaused = false;
		let persistedRequestsQueueRunning = false;

Network: Persisted Requests Queue - clear persisted requests after the response #6556

Network: Persisted Requests Queue - clear persisted requests after the response #6556

Conversation

kidroca commented Dec 1, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

kidroca left a comment

Choose a reason for hiding this comment

kidroca Dec 1, 2021

Choose a reason for hiding this comment

Network.js

kidroca Dec 1, 2021

Choose a reason for hiding this comment

kidroca Dec 1, 2021

Choose a reason for hiding this comment

kidroca Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

johnmlee101 commented Dec 9, 2021

roryabraham Dec 14, 2021

Choose a reason for hiding this comment

kidroca Dec 14, 2021

Choose a reason for hiding this comment

roryabraham commented Dec 14, 2021

kidroca commented Dec 17, 2021

mallenexpensify commented Jan 6, 2022

kidroca commented Jan 14, 2022

kidroca commented Jan 26, 2022

kidroca commented Jan 26, 2022

kidroca commented Jan 28, 2022

johnmlee101 commented Jan 28, 2022

roryabraham commented Jan 28, 2022

kidroca commented Jan 31, 2022

kidroca commented Jan 31, 2022

OSBotify commented Jan 31, 2022

OSBotify commented Feb 1, 2022

OSBotify commented Feb 2, 2022

sobitneupane Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

kidroca Feb 7, 2023

Choose a reason for hiding this comment

iwiznia Jun 5, 2023

Choose a reason for hiding this comment

kidroca commented Dec 1, 2021 •

edited

Loading

kidroca Dec 1, 2021 •

edited

Loading

sobitneupane Feb 7, 2023 •

edited

Loading