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 27 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
14 changes: 6 additions & 8 deletions src/ONYXKEYS.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,16 +243,14 @@ export default {
// Experimental memory only Onyx mode flag
IS_USING_MEMORY_ONLY_KEYS: 'isUsingMemoryOnlyKeys',

// 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',
Copy link
Member

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

Copy link
Contributor Author

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).


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

// The ID of the previous Onyx update that was applied to this client
PREVIOUS_UPDATE_ID: 'onyxUpdatesPreviousUpdateID',
},
// The access token to be used with the Mapbox library
MAPBOX_ACCESS_TOKEN: 'mapboxAccessToken',

// 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 @@ -70,7 +71,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
4 changes: 4 additions & 0 deletions src/libs/Middleware/SaveResponseInOnyx.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Onyx from 'react-native-onyx';
import CONST from '../../CONST';
import * as QueuedOnyxUpdates from '../actions/QueuedOnyxUpdates';
import * as OnyxUpdates from '../actions/OnyxUpdates';

/**
* @param {Promise} response
Expand All @@ -14,6 +15,9 @@ function SaveResponseInOnyx(response, request) {
return;
}

// Save the update IDs to Onyx so they can be used to fetch incremental updates if the client gets out of sync from the server
OnyxUpdates.saveUpdateIDs(Number(responseData.lastUpdateID || 0), Number(responseData.previousUpdateID || 0));

// 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;
Expand Down
17 changes: 8 additions & 9 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ const propTypes = {
/** Opt-in experimental mode that prevents certain Onyx keys from persisting to disk */
isUsingMemoryOnlyKeys: PropTypes.bool,

/** The last Onyx update ID that is stored in Onyx (used for getting incremental updates when reconnecting) */
onyxUpdatesLastUpdateID: PropTypes.number,
/** The last Onyx update ID was applied to the client */
lastUpdateIDAppliedToClient: PropTypes.number,

...windowDimensionsPropTypes,
};
Expand All @@ -108,7 +108,7 @@ const defaultProps = {
email: null,
},
lastOpenedPublicRoomID: null,
onyxUpdatesLastUpdateID: 0,
lastUpdateIDAppliedToClient: null,
};

class AuthScreens extends React.Component {
Expand All @@ -120,7 +120,7 @@ class AuthScreens extends React.Component {

componentDidMount() {
NetworkConnection.listenForReconnect();
NetworkConnection.onReconnect(() => App.reconnectApp(this.props.onyxUpdatesLastUpdateID));
NetworkConnection.onReconnect(() => App.reconnectApp(this.props.lastUpdateIDAppliedToClient));
PusherConnectionManager.init();
Pusher.init({
appKey: CONFIG.PUSHER.APP_KEY,
Expand All @@ -131,16 +131,15 @@ class AuthScreens extends React.Component {
});

// If we are on this screen then we are "logged in", but the user might not have "just logged in". They could be reopening the app
// or returning from background. If so, we'll assume they have some app data already and we can call
// reconnectApp(onyxUpdatesLastUpdateID) instead of openApp().
// or returning from background. If so, we'll assume they have some app data already and we can call reconnectApp() instead of openApp().
// Note: If a Guide has enabled the memory only key mode then we do want to run OpenApp as their app will not be rehydrated with
// the correct state on refresh. They are explicitly opting out of storing data they would need (i.e. reports_) to take advantage of
// the optimizations performed during ReconnectApp.
const shouldGetAllData = this.props.isUsingMemoryOnlyKeys || SessionUtils.didUserLogInDuringSession();
if (shouldGetAllData) {
App.openApp();
} else {
App.reconnectApp(this.props.onyxUpdatesLastUpdateID);
App.reconnectApp(this.props.lastUpdateIDAppliedToClient);
}

App.setUpPoliciesAndNavigate(this.props.session, !this.props.isSmallScreenWidth);
Expand Down Expand Up @@ -329,8 +328,8 @@ export default compose(
isUsingMemoryOnlyKeys: {
key: ONYXKEYS.IS_USING_MEMORY_ONLY_KEYS,
},
onyxUpdatesLastUpdateID: {
key: ONYXKEYS.ONYX_UPDATES.LAST_UPDATE_ID,
lastUpdateIDAppliedToClient: {
key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT,
},
}),
)(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: 4 additions & 1 deletion src/libs/Pusher/pusher.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 single JSON event data from Pusher', {error: err, eventData});
return;
}
if (data.id === undefined || data.chunk === undefined || data.final === undefined) {
Expand Down Expand Up @@ -172,6 +172,9 @@ function bindEventToChannel(channel, eventName, eventCallback = () => {}) {
error: err,
eventData: chunkedEvent.chunks.join(''),
});

// Using console.error is helpful here because it will print a usable stack trace to the console to debug where the error comes from
console.error(err);
danieldoglas marked this conversation as resolved.
Show resolved Hide resolved
}

delete chunkedDataEvents[data.id];
Expand Down
71 changes: 64 additions & 7 deletions src/libs/actions/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -204,14 +203,72 @@ 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),
});

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) {
Copy link
Member

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.

Copy link
Contributor Author

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 😄

console.debug('[OnyxUpdates] Gap detected in update IDs so fetching incremental updates');
Log.info('Gap detected in update IDs from server so fetching incremental updates', true, {
lastUpdateIDFromServer,
previousUpdateIDFromServer,
lastUpdateIDAppliedToClient,
});
getMissingOnyxUpdates(lastUpdateIDAppliedToClient, lastUpdateIDFromServer);
}

if (lastUpdateIDFromServer > lastUpdateIDAppliedToClient) {
// 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);
}
},
});

/**
* 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.
Expand Down
22 changes: 22 additions & 0 deletions src/libs/actions/OnyxUpdates.js
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};
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
17 changes: 2 additions & 15 deletions src/libs/actions/User.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import * as ReportActionsUtils from '../ReportActionsUtils';
import * as ErrorUtils from '../ErrorUtils';
import * as Session from './Session';
import * as PersonalDetails from './PersonalDetails';
import * as OnyxUpdates from './OnyxUpdates';

let currentUserAccountID = '';
let currentEmail = '';
Expand Down Expand Up @@ -553,21 +554,7 @@ function subscribeToUserEvents() {
updates = pushJSON;
} else {
updates = pushJSON.updates;

// Not always we'll have the lastUpdateID and previousUpdateID properties in the pusher update
// 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),
});
} else {
console.debug('[OnyxUpdates] No lastUpdateID and previousUpdateID provided');
}
OnyxUpdates.saveUpdateIDs(Number(pushJSON.lastUpdateID || 0), Number(pushJSON.previousUpdateID || 0));
}
_.each(updates, (multipleEvent) => {
PusherUtils.triggerMultiEventHandler(multipleEvent.eventType, multipleEvent.data);
Expand Down