Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Network: Persisted Requests Queue - clear persisted requests after the response #6556
Network: Persisted Requests Queue - clear persisted requests after the response #6556
Changes from 10 commits
cd23f5f
bbf477a
136423b
ef1c058
4a5f7d0
6a61ea1
5cd8b4a
729d4e6
66c8c0b
0746818
62b4208
0ffab79
32adfa0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was originally part of the network "forEach" here:
Network.js
Here (addDefaultParameters) seems to be a better place for this logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:or
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name comes from this usagage and readability reasons
This method is starting something rather than making it not paused
vs
running is the "special case" for this queue, while paused is the special case for the regular queue
I'm reluctant to refactor
isQueuePaused
toisMainQueueRunning
it would change usages and might affect their intent as well, also it kind a sound like we're online tooThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have remaining requests when
Not sure if we should capture that in a comment or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now exported from
CONST.NETWORK.PROCESS_REQUEST_DELAY_MS
and usages updatedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
The PR that fixes the issue: #14608
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 correctlyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't have done this. Context https://expensify.slack.com/archives/C01GTK53T8Q/p1685822789307339
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)