Skip to content
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

Merged
merged 28 commits into from
Aug 11, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
e6ab4fa
Add param for putting request at the front of the queue
tgolen Jul 26, 2023
4149570
Add new param for prioritizing a request
tgolen Jul 26, 2023
8bff72f
Change param name sent to API
tgolen Jul 26, 2023
dab8582
Fetch incremental updates when receiving pusher events
tgolen Jul 26, 2023
297c792
Merge branch 'main' into tgolen-priorityRequestQueue
tgolen Aug 7, 2023
1abb3ff
Cast values to ints
tgolen Aug 7, 2023
276ad1e
Explain the check for incremental updates
tgolen Aug 7, 2023
38a5261
Reduce the values stored in Onyx
tgolen Aug 7, 2023
4b25d33
Fix key name
tgolen Aug 7, 2023
0d1f2a7
Fix undefined array errors
tgolen Aug 7, 2023
0708d43
Compare IDs
tgolen Aug 7, 2023
b9c2781
Add a server log when a gap is detected
tgolen Aug 7, 2023
c15730d
Merge branch 'main' into tgolen-priorityRequestQueue
tgolen Aug 7, 2023
f89a71e
Merge branch 'main' into tgolen-priorityRequestQueue
tgolen Aug 8, 2023
9cc2132
Add new API method for fetching missed onyx updates
tgolen Aug 8, 2023
338618d
Move gap detection to a separate file
tgolen Aug 8, 2023
fb3ba07
Start moving logic to a separate method
tgolen Aug 8, 2023
b853201
Add logic for detecting missing updates
tgolen Aug 8, 2023
2bd30e8
Fix error and return a promise
tgolen Aug 8, 2023
35bb482
Remove circular dependency
tgolen Aug 8, 2023
09c7327
Remove circular dependencies
tgolen Aug 9, 2023
f17e3f3
Add comments and clean up code
tgolen Aug 9, 2023
021e41b
Remove unused import
tgolen Aug 9, 2023
d77564c
Correct docs, update comments
tgolen Aug 9, 2023
6a25f77
Correct comments and improve logs
tgolen Aug 10, 2023
2eb7c87
Remove unnecessary method
tgolen Aug 10, 2023
7934f37
Remove unnecessary local variables
tgolen Aug 10, 2023
636f26c
Rename onyx key
tgolen Aug 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions src/ONYXKEYS.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,17 +243,12 @@ export default {
// Experimental memory only Onyx mode flag
IS_USING_MEMORY_ONLY_KEYS: 'isUsingMemoryOnlyKeys',

// The ID of the last Onyx update that was applied to this client
ONYX_UPDATES_LAST_UPDATE_ID: 'onyxUpdatesLastUpdateID',

// The access token to be used with the Mapbox library
MAPBOX_ACCESS_TOKEN: 'mapboxAccessToken',

ONYX_UPDATES: {
// The ID of the last Onyx update that was applied to this client
LAST_UPDATE_ID: 'onyxUpdatesLastUpdateID',

// The ID of the previous Onyx update that was applied to this client
PREVIOUS_UPDATE_ID: 'onyxUpdatesPreviousUpdateID',
},

// Manual request tab selector
SELECTED_TAB: 'selectedTab',

Expand Down
5 changes: 3 additions & 2 deletions src/libs/API.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ Request.use(Middleware.SaveResponseInOnyx);
* @param {Object} [onyxData.optimisticData] - Onyx instructions that will be passed to Onyx.update() before the request is made.
* @param {Object} [onyxData.successData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode === 200.
* @param {Object} [onyxData.failureData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode !== 200.
* @param {Boolean} [prioritizeRequest] Whether or not the request should be prioritized at the front of the queue or placed onto the back of the queue
*/
function write(command, apiCommandParameters = {}, onyxData = {}) {
function write(command, apiCommandParameters = {}, onyxData = {}, prioritizeRequest = false) {
Log.info('Called API write', false, {command, ...apiCommandParameters});

// Optimistically update Onyx
Expand Down Expand Up @@ -66,7 +67,7 @@ function write(command, apiCommandParameters = {}, onyxData = {}) {
};

// Write commands can be saved and retried, so push it to the SequentialQueue
SequentialQueue.push(request);
SequentialQueue.push(request, prioritizeRequest);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ export default compose(
key: ONYXKEYS.IS_USING_MEMORY_ONLY_KEYS,
},
onyxUpdatesLastUpdateID: {
key: ONYXKEYS.ONYX_UPDATES.LAST_UPDATE_ID,
key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID,
},
}),
)(AuthScreens);
5 changes: 3 additions & 2 deletions src/libs/Network/SequentialQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,11 @@ NetworkStore.onReconnection(flush);

/**
* @param {Object} request
* @param {Boolean} [front] whether or not the request should be placed in the front of the queue
*/
function push(request) {
function push(request, front = false) {
// Add request to Persisted Requests so that it can be retried if it fails
PersistedRequests.save([request]);
PersistedRequests.save([request], front);

// If we are offline we don't need to trigger the queue to empty as it will happen when we come back online
if (NetworkStore.isOffline()) {
Expand Down
5 changes: 3 additions & 2 deletions src/libs/actions/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,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
* @param {Boolean} [prioritizeRequest] whether or not the request should be put at the front of the sequential queue before other requests
*/
function reconnectApp(updateIDFrom = 0, updateIDTo = 0) {
function reconnectApp(updateIDFrom = 0, updateIDTo = 0, prioritizeRequest = false) {
console.debug(`[OnyxUpdates] App reconnecting with updateIDFrom: ${updateIDFrom} and updateIDTo: ${updateIDTo}`);
getPolicyParamsForOpenOrReconnect().then((policyParams) => {
const params = {...policyParams};
Expand All @@ -208,7 +209,7 @@ function reconnectApp(updateIDFrom = 0, updateIDTo = 0) {
params.updateIDTo = updateIDTo;
}

API.write('ReconnectApp', params, getOnyxDataForOpenOrReconnect());
API.write('ReconnectApp', params, getOnyxDataForOpenOrReconnect(), prioritizeRequest);
});
}

Expand Down
9 changes: 7 additions & 2 deletions src/libs/actions/PersistedRequests.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,14 @@ function clear() {

/**
* @param {Array} requestsToPersist
* @param {Boolean} [front] whether or not the request should go in the front of the queue
*/
function save(requestsToPersist) {
persistedRequests = persistedRequests.concat(requestsToPersist);
function save(requestsToPersist, front = false) {
if (persistedRequests.length) {
persistedRequests = front ? persistedRequests.unshift(...requestsToPersist) : persistedRequests.concat(requestsToPersist);
} else {
persistedRequests = requestsToPersist;
}
Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
}

Expand Down
38 changes: 30 additions & 8 deletions src/libs/actions/User.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import * as ReportActionsUtils from '../ReportActionsUtils';
import * as ErrorUtils from '../ErrorUtils';
import * as Session from './Session';
import * as PersonalDetails from './PersonalDetails';
import * as App from './App';
import Log from '../Log';

let currentUserAccountID = '';
let currentEmail = '';
Expand All @@ -41,6 +43,12 @@ Onyx.connect({
},
});

let onyxUpdatesLastUpdateID;
Onyx.connect({
key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID,
callback: (val) => (onyxUpdatesLastUpdateID = val),
});

/**
* Attempt to close the user's account
*
Expand Down Expand Up @@ -558,15 +566,29 @@ function subscribeToUserEvents() {
// until we finish the migration to reliable updates. So let's check it before actually updating
// the properties in Onyx
if (pushJSON.lastUpdateID && pushJSON.previousUpdateID) {
console.debug('[OnyxUpdates] Received lastUpdateID from pusher', pushJSON.lastUpdateID);
console.debug('[OnyxUpdates] Received previousUpdateID from pusher', pushJSON.previousUpdateID);
// Store these values in Onyx to allow App.reconnectApp() to fetch incremental updates from the server when a previous session is being reconnected to.
Onyx.multiSet({
[ONYXKEYS.ONYX_UPDATES.LAST_UPDATE_ID]: Number(pushJSON.lastUpdateID || 0),
[ONYXKEYS.ONYX_UPDATES.PREVIOUS_UPDATE_ID]: Number(pushJSON.previousUpdateID || 0),
});
const pushJSONLastUpdateID = Number(pushJSON.lastUpdateID || 0);
const pushJSONPreviousUpdateID = Number(pushJSON.previousUpdateID || 0);

console.debug('[OnyxUpdates] Received lastUpdateID from pusher', pushJSONLastUpdateID);
console.debug('[OnyxUpdates] Received previousUpdateID from pusher', pushJSONPreviousUpdateID);
console.debug('[OnyxUpdates] The lastUpdateID the client received was', onyxUpdatesLastUpdateID);

Copy link
Contributor

@danieldoglas danieldoglas Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update!

// Store this value in Onyx to allow AuthScreens to fetch incremental updates from the server when a previous session is being reconnected to.
Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID, pushJSONLastUpdateID);

// The previous update from the server does not match the last update the client got which means the client is missing some updates.
// ReconnectApp will fetch updates starting from the last update this client got and going to the last update the server sent.
if (onyxUpdatesLastUpdateID < pushJSONPreviousUpdateID) {
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, {
lastUpdateIDFromPusher: pushJSONLastUpdateID,
previousUpdateIDFromPusher: pushJSONPreviousUpdateID,
lastUpdateIDOnClient: onyxUpdatesLastUpdateID,
});
App.reconnectApp(onyxUpdatesLastUpdateID, pushJSONLastUpdateID, true);
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

} else {
console.debug('[OnyxUpdates] No lastUpdateID and previousUpdateID provided');
console.debug('[OnyxUpdates] No lastUpdateID and previousUpdateID provided from Pusher');
}
}
_.each(updates, (multipleEvent) => {
Expand Down
Loading