-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Allow priority requests to go to the front of the sequential queue #23669
Conversation
@thesahindia Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@thesahindia @ One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
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.
PR looks great, but there's one change we should do for clarity.
I tested and everything worked as we expected for this PR. let's just call the correct method and we should be good!
src/libs/actions/User.js
Outdated
App.reconnectApp(onyxUpdatesLastUpdateID, pushJSONLastUpdateID, true); | ||
} |
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.
@tgolen I think you missed this, but you created a GetMissingOnyxMessages method in the API to use exactly on this situation instead of using ReconnectApp. I think we should use it here for clarity.
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.
OK, I will switch to using this.
src/libs/actions/User.js
Outdated
console.debug('[OnyxUpdates] The lastUpdateID the client received was', onyxUpdatesLastUpdateID); | ||
|
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.
console.debug('[OnyxUpdates] The lastUpdateID the client received was', onyxUpdatesLastUpdateID); | |
console.debug('[OnyxUpdates] The lastUpdateID the client had was', onyxUpdatesLastUpdateID); | |
Just for clarity, I was confused if this was the one I received or the one we had when just looking at the logs
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.
I will update!
OK, I had to do a pretty major rewrite in order to DRY the code between Pusher responses and HTTPS responses. The problem was that the middleware for I got around the problem by using Onyx keys and a callback function to listen for when the updateIDs sent from the server have changed. |
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.
Looking great, just a few NAB comments and one thing we need to fix!
src/libs/Pusher/pusher.js
Outdated
@@ -136,7 +136,7 @@ function bindEventToChannel(channel, eventName, eventCallback = () => {}) { | |||
try { | |||
data = _.isObject(eventData) ? eventData : JSON.parse(eventData); | |||
} catch (err) { | |||
Log.alert('[Pusher] Unable to parse JSON response from Pusher', {error: err, eventData}); | |||
Log.alert('[Pusher] Unable to parse JSON response from Pusher 1', {error: err, eventData}); |
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.
NAB: is this intentional?
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.
Sort of, yeah, I can do it better though. The same log happens from two different places and it wasn't possible to tell from which spot I was seeing an error.
src/libs/Pusher/pusher.js
Outdated
@@ -168,10 +168,11 @@ function bindEventToChannel(channel, eventName, eventCallback = () => {}) { | |||
try { | |||
eventCallback(JSON.parse(chunkedEvent.chunks.join(''))); | |||
} catch (err) { | |||
Log.alert('[Pusher] Unable to parse chunked JSON response from Pusher', { | |||
Log.alert('[Pusher] Unable to parse chunked JSON response from Pusher 2', { |
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.
NAB: is this intentional?
src/libs/actions/App.js
Outdated
// getMissingOnyxUpdates will fetch updates starting from the last update this client got and going to the last update the server sent. | ||
if (lastUpdateIDAppliedToClient && previousUpdateIDFromServer && lastUpdateIDAppliedToClient < previousUpdateIDFromServer) { | ||
console.debug('[OnyxUpdates] Gap detected in update IDs so fetching incremental updates'); | ||
Log.info('Gap detected in update IDs from Pusher so fetching incremental updates', true, { |
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.
Log.info('Gap detected in update IDs from Pusher so fetching incremental updates', true, { | |
Log.info('Gap detected in update IDs from server so fetching incremental updates', true, { |
NAB: Just suggest we change this from server since this will be triggered from both Pusher and HTTPs requests
@@ -16,7 +35,7 @@ function SaveResponseInOnyx(response, request) { | |||
|
|||
// For most requests we can immediately update Onyx. For write requests we queue the updates and apply them after the sequential queue has flushed to prevent a replay effect in | |||
// the UI. See https://github.com/Expensify/App/issues/12775 for more info. | |||
const updateHandler = request.data.apiRequestType === CONST.API_REQUEST_TYPE.WRITE ? QueuedOnyxUpdates.queueOnyxUpdates : Onyx.update; | |||
const updateHandler = request.data.apiRequestType === CONST.API_REQUEST_TYPE.WRITE ? QueuedOnyxUpdates.queueOnyxUpdates : updateOnyx; |
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.
I think we should change both requests here, not only the usage of Onyx.Update
.
I say that because write requests are the ones that actually generate an onyx response for all clients. If you call OpenInitialSettings, which is a read, we don't send data through onyx to it since it's just for the local client, it won't change anything. So that's not written in onyxUpdates.
But if you call OpenReport, which is a write and returns lastUpdateID
and previousUpdateID
(since we update the lastRead property of the report), we're not saving updating those properties locally
So let's say we're sending messages to someone, then we go from report A to report B and receive a new message, that would trigger GetOnyxUpdates, which is not ideal.
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.
Thanks for pointing that out. Makes sense!
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.
Added a few more considerations after testing
src/libs/actions/App.js
Outdated
// Update the local values to be the same as the values stored in Onyx | ||
onyxUpdates.lastUpdateIDFromServer = lastUpdateIDFromServer || 0; | ||
onyxUpdates.previousUpdateIDFromServer = previousUpdateIDFromServer || 0; |
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.
Why? We are not using this local variables for anything
src/libs/actions/App.js
Outdated
const onyxUpdates = { | ||
lastUpdateIDFromServer: 0, | ||
previousUpdateIDFromServer: 0, | ||
}; |
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 object is set and updated, but we never read from it. we can remove it since we use the values directly from Onyx in the function below
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.
I rewrote this so many times yesterday I got tunnelvision. Thanks for your fresh eyes on this! I've cleaned it up to remove this.
src/libs/actions/App.js
Outdated
Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, lastUpdateIDFromServer || 0); | ||
}, |
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.
From your PR I was able to see something I wasn't seeing before: We also send GetOnyxUpdate responses through Pusher, so we're updating the current client and sending those updates to other clients. I think that's fine since if we're sending it to all clients, the changes the clients are updated are bigger.
What that does is:
- Client A and B are using account 1
- Client A has OnyxUpdatesLastUpdateIDAppliedToClient with 51
- Client B as OnyxUpdatesLastUpdateIDAppliedToClient with 47
- Account 2 sends a message to Account 1 which will be updateID 55
- Client B for some reason doesn't receive it
- Client A receives, and calls
GetOnyxUpdates
withfrom: 51
andto: 55
- Client B, that has it's connection working now, receives the
GetOnyxUpdates
from Pusher, sees that the previousUpdateID in that update is bigger than 47, and calls GetOnyxUpdates with valuesfrom: 47
andto: 51
, and gets up to date.
This works OK!
Now we have an edge case that happened when I was testing.
- Client A and B are using account 1
- Client B as OnyxUpdatesLastUpdateIDAppliedToClient with 47
- Account 2 sends a message to Account 1 which will be updateID 55
- Client A receives it instantly and now has OnyxUpdatesLastUpdateIDAppliedToClient with 55. Then it does some actions, and that number rises to 60
- Client B for some reason doesn't receive those
- Client B connects and receives update 60. It notices it didn't have 47-59, and does a
GetOnyxUpdates
with those - Client A receives those updates since we're testing. It won't apply those, since the ID is less than the one it currently has BUT it will save
OnyxUpdatesLastUpdateIDAppliedToClient
with lastUpdateID 58 that was sent in the pusher update body. - So next time it tries to do anything, it will call
GetMissingOnyxUpdates
again
I think we solve that by adding this to the if statement above.
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.
OK, to make sure I understand your proposed change clearly, you are suggesting to only update OnyxUpdatesLastUpdateIDAppliedToClient
if the lastUpdateIDFromServer
is larger. Right? That way the value will never decrease, only increase.
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.
Yes, that's correct!
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.
LGTM, thanks!
@thesahindia can you please review this? |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-08-11.at.15.12.20.movScreen.Recording.2023-08-11.at.15.09.32.movMobile Web - SafariScreen.Recording.2023-08-11.at.16.36.27.movDesktopScreen.Recording.2023-08-11.at.16.36.27.moviOS9f60bd04-d856-4e20-8e5c-7e945be19055.mp4 |
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.
LGTM!
Did a general test on the app and confirmed that everything works as intended
|
||
// If the previous update from the server does not match the last update the client got, then the client is missing some updates. | ||
// getMissingOnyxUpdates will fetch updates starting from the last update this client got and going to the last update the server sent. | ||
if (lastUpdateIDAppliedToClient && previousUpdateIDFromServer && lastUpdateIDAppliedToClient < previousUpdateIDFromServer) { |
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.
if expensify goes to mars and beyond, we mayy have more than 2^63 - 1
server updates for users by then.
and then they'll stop getting any updates.
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.
If that's the case, I'll happily come back here and update the logic 😄
src/ONYXKEYS.js
Outdated
// The access token to be used with the Mapbox library | ||
MAPBOX_ACCESS_TOKEN: 'mapboxAccessToken', | ||
// Information about the onyx updates IDs that were received from the server | ||
ONYX_UPDATES: 'onyxUpdates', |
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.
NAB: These are server updates only. I would have named them ONYX_SERVER_UPDATES
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.
Updated branch and renamed this to ONYX_UPDATES_FROM_SERVER
following your suggestion (I think the name matches the other key better).
@cristipaval Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
🎯 @rushatgabhane, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #24418. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/danieldoglas in version: 1.3.54-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/danieldoglas in version: 1.3.54-0 🚀
|
@tgolen are we ok to check it off? As its internal |
Yep, you can check it off. We'll be manually testing it this week |
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.54-13 🚀
|
Details
When the app reconnects to the network and needs to get incremental OnyxUpdates from the server, that request needs to be prioritized at the front of the sequential queue so that it is processed immediately and the pending updates are applied to the most recent data from the server.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/299547
Tests
Note: This can only be tested on Web by internal engineers for now
[OnyxUpdates]
For C+ tester
Do a general test on the app and confirm that everything still works as intended.
Offline tests
None
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Note: this is only testable on web
Mobile Web - Safari
Note: this is only testable on web
Desktop
Note: this is only testable on web
iOS
Note: this is only testable on web
Android
Note: this is only testable on web