-
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
Changes from 24 commits
e6ab4fa
4149570
8bff72f
dab8582
297c792
1abb3ff
276ad1e
38a5261
4b25d33
0d1f2a7
0708d43
b9c2781
c15730d
f89a71e
9cc2132
338618d
fb3ba07
b853201
2bd30e8
35bb482
09c7327
f17e3f3
021e41b
d77564c
6a25f77
2eb7c87
7934f37
636f26c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,25 @@ | ||
import Onyx from 'react-native-onyx'; | ||
import CONST from '../../CONST'; | ||
import * as QueuedOnyxUpdates from '../actions/QueuedOnyxUpdates'; | ||
import * as OnyxUpdates from '../actions/OnyxUpdates'; | ||
|
||
/** | ||
* @param {Object} updates | ||
* @param {Object[]} [updates.onyxData] an array of Onyx update instruction objects | ||
* @param {Number} [updates.previousUpdateID] | ||
* @param {Number} [updates.lastUpdateID] | ||
* @returns {Promise} | ||
*/ | ||
function updateOnyx(updates) { | ||
// `updates.onyxData` only exists in the latest format of the OnyxUpdates provided by the RELIABLE_UPDATES beta. | ||
// If the value doesn't exist, then onyx is updated with the old format of updates. | ||
if (!updates.onyxData) { | ||
return Onyx.update(updates); | ||
} | ||
|
||
OnyxUpdates.saveUpdateIDs(Number(updates.lastUpdateID || 0), Number(updates.previousUpdateID || 0)); | ||
return Onyx.update(updates.onyxData); | ||
} | ||
|
||
/** | ||
* @param {Promise} response | ||
|
@@ -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 commentThe 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 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 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 commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing that out. Makes sense! |
||
|
||
// First apply any onyx data updates that are being sent back from the API. We wait for this to complete and then | ||
// apply successData or failureData. This ensures that we do not update any pending, loading, or other UI states contained | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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. |
||
return; | ||
} | ||
if (data.id === undefined || data.chunk === undefined || data.final === undefined) { | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. NAB: is this intentional? |
||
error: err, | ||
eventData: chunkedEvent.chunks.join(''), | ||
}); | ||
console.error(err); | ||
danieldoglas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
delete chunkedDataEvents[data.id]; | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -182,10 +182,9 @@ function openApp() { | |||||
/** | ||||||
* Fetches data when the app reconnects to the network | ||||||
* @param {Number} [updateIDFrom] the ID of the Onyx update that we want to start fetching from | ||||||
* @param {Number} [updateIDTo] the ID of the Onyx update that we want to fetch up to | ||||||
*/ | ||||||
function reconnectApp(updateIDFrom = 0, updateIDTo = 0) { | ||||||
console.debug(`[OnyxUpdates] App reconnecting with updateIDFrom: ${updateIDFrom} and updateIDTo: ${updateIDTo}`); | ||||||
function reconnectApp(updateIDFrom = 0) { | ||||||
console.debug(`[OnyxUpdates] App reconnecting with updateIDFrom: ${updateIDFrom}`); | ||||||
getPolicyParamsForOpenOrReconnect().then((policyParams) => { | ||||||
const params = {...policyParams}; | ||||||
|
||||||
|
@@ -204,14 +203,79 @@ function reconnectApp(updateIDFrom = 0, updateIDTo = 0) { | |||||
params.updateIDFrom = updateIDFrom; | ||||||
} | ||||||
|
||||||
if (updateIDTo) { | ||||||
params.updateIDTo = updateIDTo; | ||||||
} | ||||||
|
||||||
API.write('ReconnectApp', params, getOnyxDataForOpenOrReconnect()); | ||||||
}); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Fetches data when the client has discovered it missed some Onyx updates from the server | ||||||
* @param {Number} [updateIDFrom] the ID of the Onyx update that we want to start fetching from | ||||||
* @param {Number} [updateIDTo] the ID of the Onyx update that we want to fetch up to | ||||||
*/ | ||||||
function getMissingOnyxUpdates(updateIDFrom = 0, updateIDTo = 0) { | ||||||
console.debug(`[OnyxUpdates] Fetching missing updates updateIDFrom: ${updateIDFrom} and updateIDTo: ${updateIDTo}`); | ||||||
|
||||||
API.write( | ||||||
'GetMissingOnyxMessages', | ||||||
{ | ||||||
updateIDFrom, | ||||||
updateIDTo, | ||||||
}, | ||||||
getOnyxDataForOpenOrReconnect(), | ||||||
|
||||||
// Set this to true so that the request will be prioritized at the front of the sequential queue | ||||||
true, | ||||||
); | ||||||
} | ||||||
|
||||||
// The next 40ish lines of code are used for detecting when there is a gap of OnyxUpdates between what was last applied to the client and the updates the server has. | ||||||
// When a gap is detected, the missing updates are fetched from the API. | ||||||
|
||||||
// These key needs to be separate from ONYXKEYS.ONYX_UPDATES so that it can be updated without triggering the callback when the server IDs are updated | ||||||
let lastUpdateIDAppliedToClient = 0; | ||||||
Onyx.connect({ | ||||||
key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, | ||||||
callback: (val) => (lastUpdateIDAppliedToClient = val), | ||||||
}); | ||||||
|
||||||
// This object is stored in Onyx and the callback is triggered anytime new update IDs are received either from Pusher or from HTTPs (it was the only way to keep the code DRY and to prevent circular dependencies) | ||||||
const onyxUpdates = { | ||||||
lastUpdateIDFromServer: 0, | ||||||
previousUpdateIDFromServer: 0, | ||||||
}; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||||||
Onyx.connect({ | ||||||
key: ONYXKEYS.ONYX_UPDATES, | ||||||
callback: (val) => { | ||||||
if (!val) { | ||||||
return; | ||||||
} | ||||||
|
||||||
const {lastUpdateIDFromServer, previousUpdateIDFromServer} = val; | ||||||
console.debug('[OnyxUpdates] Received lastUpdateID from server', lastUpdateIDFromServer); | ||||||
console.debug('[OnyxUpdates] Received previousUpdateID from server', previousUpdateIDFromServer); | ||||||
console.debug('[OnyxUpdates] Last update ID applied to the client', lastUpdateIDAppliedToClient); | ||||||
|
||||||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. if expensify goes to mars and beyond, we mayy have more than and then they'll stop getting any updates. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😄 |
||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
NAB: Just suggest we change this from server since this will be triggered from both Pusher and HTTPs requests |
||||||
lastUpdateIDFromServer, | ||||||
previousUpdateIDFromServer, | ||||||
lastUpdateIDAppliedToClient, | ||||||
}); | ||||||
getMissingOnyxUpdates(lastUpdateIDAppliedToClient, lastUpdateIDFromServer); | ||||||
} | ||||||
|
||||||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. Why? We are not using this local variables for anything |
||||||
|
||||||
// Update this value so that it matches what was just received from the server | ||||||
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 commentThe 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:
This works OK! Now we have an edge case that happened when I was testing.
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's correct! |
||||||
}); | ||||||
|
||||||
/** | ||||||
* This promise is used so that deeplink component know when a transition is end. | ||||||
* This is necessary because we want to begin deeplink redirection after the transition is end. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import Onyx from 'react-native-onyx'; | ||
import ONYXKEYS from '../../ONYXKEYS'; | ||
|
||
/** | ||
* | ||
* @param {Number} [lastUpdateID] | ||
* @param {Number} [previousUpdateID] | ||
*/ | ||
function saveUpdateIDs(lastUpdateID = 0, previousUpdateID = 0) { | ||
// Return early if there were no updateIDs | ||
if (!lastUpdateID) { | ||
return; | ||
} | ||
|
||
Onyx.merge(ONYXKEYS.ONYX_UPDATES, { | ||
lastUpdateIDFromServer: lastUpdateID, | ||
previousUpdateIDFromServer: previousUpdateID, | ||
}); | ||
} | ||
|
||
// eslint-disable-next-line import/prefer-default-export | ||
export {saveUpdateIDs}; |
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).