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

fixed messages are repeated on page refresh & process queue only when client is leader #2703

Merged
merged 14 commits into from
Aug 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 14 additions & 1 deletion src/libs/ActiveClientManager/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@ const clientID = Str.guid();
const maxClients = 20;

let activeClients;
let isInitialized;

// Keeps track of the ActiveClientManager's readiness in one place
// so that multiple calls of isReady resolve the same promise
const isInitializedPromise = new Promise((resolve) => {
isInitialized = resolve;
});

Onyx.connect({
key: ONYXKEYS.ACTIVE_CLIENTS,
callback: (val) => {
Expand All @@ -22,7 +30,11 @@ Onyx.connect({
* Add our client ID to the list of active IDs
*/
function init() {
Onyx.merge(ONYXKEYS.ACTIVE_CLIENTS, [clientID]);
Onyx.merge(ONYXKEYS.ACTIVE_CLIENTS, [clientID]).then(isInitialized);
}

function isReady() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This way we don't need to keep the promise in memory. I used Promise over your suggested callback method onReady(() => {}). We can just use promise in multiple places for multiple tasks. The callback would require an array to be implemented for that purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I want to do it the way I've suggested for consistency reasons. It's not that your idea won't work, it's that we don't have many patterns like this and it's pretty unusual to see.

We can just use promise in multiple places for multiple tasks. The callback would require an array to be implemented for that purpose.

There is no need to do this as only one thing needs to know whether we have "initialized".

return isInitializedPromise;
}

/**
Expand All @@ -37,4 +49,5 @@ function isClientTheLeader() {
export {
init,
isClientTheLeader,
isReady,
};
4 changes: 4 additions & 0 deletions src/libs/ActiveClientManager/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@ function init() {}
function isClientTheLeader() {
return true;
}
function isReady() {
return Promise.resolve();
}

export {
init,
isReady,
isClientTheLeader,
};
103 changes: 83 additions & 20 deletions src/libs/Network.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import lodashGet from 'lodash/get';
import Onyx from 'react-native-onyx';
import HttpUtils from './HttpUtils';
import ONYXKEYS from '../ONYXKEYS';
import * as ActiveClientManager from './ActiveClientManager';
import CONST from '../CONST';

let isQueuePaused = false;
Expand All @@ -20,30 +21,73 @@ let enhanceParameters;
let onResponse = () => {};
let onError = () => {};

let didLoadPersistedRequests;
let isOffline;

/**
* Process the offline NETWORK_REQUEST_QUEUE
* @param {Array<Object> | null} persistedRequests - Requests
*/
function processOfflineQueue(persistedRequests) {
// NETWORK_REQUEST_QUEUE is shared across clients, thus every client will have similiar copy of
// NETWORK_REQUEST_QUEUE. It is very important to only process the queue from leader client
// otherwise requests will be duplicated.
// We only process the persisted requests when
// a) Client is leader.
// b) User is online.
// c) requests are not already loaded,
// d) When there is at least one request
if (!ActiveClientManager.isClientTheLeader()
|| isOffline
|| didLoadPersistedRequests
|| !persistedRequests
|| !persistedRequests.length) {
return;
}

// Queue processing expects handlers but due to we are loading the requests from Storage
// we just noop them to ignore the errors.
_.each(persistedRequests, (request) => {
request.resolve = () => {};
request.reject = () => {};
});

// Merge the persisted requests with the requests in memory then clear out the queue as we only need to load
// this once when the app initializes
networkRequestQueue = [...networkRequestQueue, ...persistedRequests];
Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, []);
didLoadPersistedRequests = true;
}

// We subscribe to changes to the online/offline status of the network to determine when we should fire off API calls
// vs queueing them for later.
let isOffline;
Onyx.connect({
key: ONYXKEYS.NETWORK,
callback: val => isOffline = val && val.isOffline,
});

let didLoadPersistedRequests;
Onyx.connect({
key: ONYXKEYS.NETWORK_REQUEST_QUEUE,
callback: (persistedRequests) => {
if (didLoadPersistedRequests || !persistedRequests) {
callback: (val) => {
if (!val) {
return;
}

// Merge the persisted requests with the requests in memory then clear out the queue as we only need to load
// this once when the app initializes
networkRequestQueue = [...networkRequestQueue, ...persistedRequests];
didLoadPersistedRequests = true;
Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, []);
// Client becomes online, process the queue.
if (isOffline && !val.isOffline) {
const connection = Onyx.connect({
key: ONYXKEYS.NETWORK_REQUEST_QUEUE,
callback: processOfflineQueue,
});
Onyx.disconnect(connection);
}
isOffline = val.isOffline;
},
});

// Subscribe to NETWORK_REQUEST_QUEUE queue as soon as Client is ready
ActiveClientManager.isReady().then(() => {
Onyx.connect({
key: ONYXKEYS.NETWORK_REQUEST_QUEUE,
callback: processOfflineQueue,
});
});

// Subscribe to the user's session so we can include their email in every request and include it in the server logs
let email;
Onyx.connect({
Expand Down Expand Up @@ -107,12 +151,19 @@ function processNetworkRequestQueue() {
if (!networkRequestQueue.length) {
return;
}

// If we have a request then we need to check if it can be persisted in case we close the tab while offline
const retryableRequests = _.filter(networkRequestQueue, request => (
!request.data.doNotRetry && request.data.persist
));
Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, retryableRequests);
const retryableRequests = [];

// If we have a request then we need to check if it can be persisted in case we close the tab while offline.
// We filter persisted requests from the normal Queue to remove duplicates
networkRequestQueue = _.reject(networkRequestQueue, (request) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We filter persisted requests from the normal Queue to remove duplicates

Which duplicates are we talking about here? Can you provide an example? It's hard to visualize what you mean or why we are removing these from the normal request queue in memory.

Wouldn't this mean that if I add a comment while offline that it will be "removed" from the queue and not sent again when I come back online?

Copy link
Member Author

Choose a reason for hiding this comment

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

So Onyx.connect is called for NETWORK_REQUEST_QUEUE when we come back online which takes care of those.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link me to where this happens?

Copy link
Contributor

Choose a reason for hiding this comment

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

bump

Copy link
Member Author

@parasharrajat parasharrajat Aug 3, 2021

Choose a reason for hiding this comment

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

When a user comes online on line 37 we process the queue by fake merge which I will optimize based on the suggestion.

if (!request.data.doNotRetry && request.data.persist) {
retryableRequests.push(request);
return true;
}
});
if (retryableRequests.length) {
Onyx.merge(ONYXKEYS.NETWORK_REQUEST_QUEUE, retryableRequests);
TomatoToaster marked this conversation as resolved.
Show resolved Hide resolved
}
return;
}

Expand Down Expand Up @@ -161,6 +212,18 @@ function processNetworkRequestQueue() {
.catch(error => onError(queuedRequest, error));
});

// We should clear the NETWORK_REQUEST_QUEUE when we have loaded the persisted requests & they are processed.
// As multiple client will be sharing the same Queue and NETWORK_REQUEST_QUEUE is synchronized among clients,
// we only ask Leader client to clear the queue
if (ActiveClientManager.isClientTheLeader() && didLoadPersistedRequests) {
TomatoToaster marked this conversation as resolved.
Show resolved Hide resolved
Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, []);
}

// User could have bad connectivity and he can go offline multiple times
// thus we allow NETWORK_REQUEST_QUEUE to be processed multiple times but only after we have processed
// old requests in the NETWORK_REQUEST_QUEUE
didLoadPersistedRequests = false;

// We clear the request queue at the end by setting the queue to retryableRequests which will either have some
// requests we want to retry or an empty array
networkRequestQueue = requestsToProcessOnNextRun;
Expand Down
4 changes: 2 additions & 2 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -582,8 +582,8 @@ function updateReportWithNewAction(reportID, reportAction, notificationPreferenc

const reportActionsToMerge = {};
if (reportAction.clientID) {
// Remove the optimistic action from the report since we are about to replace it with the real one (which has
// the true sequenceNumber)
// Remove the optimistic action from the report since we are about to replace it
// with the real one (which has the true sequenceNumber)
reportActionsToMerge[reportAction.clientID] = null;
}

Expand Down