-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Network Cleanup: Isolate "persisted " queue from "main" queue #8312
Changes from 8 commits
204514d
9193948
5d61b2d
0d5285c
4ce55d6
d67fff7
e16211f
f4c1cb2
88279fe
5792e80
6d0576a
c39f2cf
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 |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import CONST from '../../CONST'; | ||
import createCallback from '../createCallback'; | ||
|
||
const [getLogger, registerLogHandler] = createCallback(); | ||
const [triggerConnectivityResumed, onConnectivityResumed] = createCallback(); | ||
const [onRequest, registerRequestHandler] = createCallback(); | ||
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 find this naming a bit confusing. We have two separate naming conventions both being used:
We should choose one and be consistent. I would recommend 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 do the above ^ in any event, but you could also take this a step further and create a NETWORK_EVENTS = {
REQUEST: 'request',
RESPONSE: 'response',
ERROR: 'error',
CONNECTIVITY_RESUMED: 'connectivityResumed',
CONNECTIVITY_CHECK: 'connectivityCheck',
}; And then refactor this lib like so: const events = _.reduce(
CONST.NETWORK_EVENTS,
(memo, eventName) => ({
...memo,
[eventName]: createCallback(),
}),
{},
);
function emit(name, payload) {
const event = events[name];
if (!event) {
throw new Error(`The ${name} network event does not exist!`);
}
const eventTrigger = event[0];
eventTrigger(payload);
}
function registerHandler(name, callback) {
const event = events[name]
if (!event) {
throw new Error(`The ${name} network event does not exist!`);
}
const registerEventHandler = event[1];
registerEventHandler(name, callback);
}
export {
emit,
registerHandler,
}; And then use the refactored lib like this: NetworkEvents.registerHandler(CONST.NETWORK_EVENTS.ERROR, (error) => Log.alert(error.message));
NetworkEvents.emit(CONST.NETWORK_EVENTS.ERROR, {message: 'Some error message'}); 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. Solid idea. Can see how it cleans stuff up. thought: It looks like we'd be able to register n handlers for a given event and that might lead one to make a mistake that they could add extra handlers proposal: Let's clean up the names for now and do a proper 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.
Sounds great! I'm happy to approve whatever level of cleanup you want to do 😄 |
||
const [onResponse, registerResponseHandler] = createCallback(); | ||
const [onError, registerErrorHandler] = createCallback(); | ||
const [triggerRecheckNeeded, registerConnectionCheckCallback] = createCallback(); | ||
|
||
/** | ||
* @returns {Function} cancel timer | ||
*/ | ||
function startRequestTimeoutTimer() { | ||
// If request is still in processing after this time, we might be offline | ||
const timerId = setTimeout(() => triggerRecheckNeeded(), CONST.NETWORK.MAX_PENDING_TIME_MS); | ||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return () => clearTimeout(timerId); | ||
} | ||
|
||
export { | ||
registerLogHandler, | ||
getLogger, | ||
triggerConnectivityResumed, | ||
onConnectivityResumed, | ||
onRequest, | ||
registerRequestHandler, | ||
onResponse, | ||
registerResponseHandler, | ||
onError, | ||
registerErrorHandler, | ||
triggerRecheckNeeded, | ||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
registerConnectionCheckCallback, | ||
startRequestTimeoutTimer, | ||
}; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,48 +2,76 @@ import lodashGet from 'lodash/get'; | |||||
import Onyx from 'react-native-onyx'; | ||||||
import _ from 'underscore'; | ||||||
import ONYXKEYS from '../../ONYXKEYS'; | ||||||
import * as NetworkEvents from './NetworkEvents'; | ||||||
|
||||||
let credentials; | ||||||
let authToken; | ||||||
let currentUserEmail; | ||||||
let networkReady = false; | ||||||
let hasReadRequiredData = false; | ||||||
let authenticating = false; | ||||||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
let isOffline = false; | ||||||
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. We can do all this later (as part of offline-first doc) if you prefer, but should we assume that we're offline unless we know otherwise?
Suggested change
Further, if we made that change, could we remove the self-proclaimed hack for
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. Good thoughts. I don't have a clear answer about which would be better from an Offline First Doc perspective and would have to weigh some pros/cons. I don't think we can remove that hack if we do this anyway as there's no guarantee we won't set |
||||||
|
||||||
/** | ||||||
* @param {Boolean} ready | ||||||
* @param {Boolean} val | ||||||
*/ | ||||||
function setIsReady(ready) { | ||||||
networkReady = ready; | ||||||
function setHasReadRequiredDataFromStorage(val) { | ||||||
hasReadRequiredData = val; | ||||||
} | ||||||
|
||||||
/** | ||||||
* This is a hack to workaround the fact that Onyx may not yet have read these values from storage by the time Network starts processing requests. | ||||||
* If the values are undefined we haven't read them yet. If they are null or have a value then we have and the network is "ready". | ||||||
*/ | ||||||
function checkRequiredDataAndSetNetworkReady() { | ||||||
function checkRequiredData() { | ||||||
if (_.isUndefined(authToken) || _.isUndefined(credentials)) { | ||||||
return; | ||||||
} | ||||||
|
||||||
setIsReady(true); | ||||||
setHasReadRequiredDataFromStorage(true); | ||||||
} | ||||||
|
||||||
Onyx.connect({ | ||||||
key: ONYXKEYS.SESSION, | ||||||
callback: (val) => { | ||||||
authToken = lodashGet(val, 'authToken', null); | ||||||
currentUserEmail = lodashGet(val, 'email', null); | ||||||
checkRequiredDataAndSetNetworkReady(); | ||||||
checkRequiredData(); | ||||||
}, | ||||||
}); | ||||||
|
||||||
Onyx.connect({ | ||||||
key: ONYXKEYS.CREDENTIALS, | ||||||
callback: (val) => { | ||||||
credentials = val || null; | ||||||
checkRequiredDataAndSetNetworkReady(); | ||||||
checkRequiredData(); | ||||||
}, | ||||||
}); | ||||||
|
||||||
// We subscribe to the online/offline status of the network to determine when we should fire off API calls | ||||||
// vs queueing them for later. | ||||||
Onyx.connect({ | ||||||
key: ONYXKEYS.NETWORK, | ||||||
callback: (network) => { | ||||||
if (!network) { | ||||||
return; | ||||||
} | ||||||
|
||||||
// Client becomes online emit connectivity resumed event | ||||||
if (isOffline && !network.isOffline) { | ||||||
NetworkEvents.triggerConnectivityResumed(); | ||||||
} | ||||||
|
||||||
isOffline = network.isOffline; | ||||||
}, | ||||||
}); | ||||||
|
||||||
/** | ||||||
* @returns {Boolean} | ||||||
*/ | ||||||
function getIsOffline() { | ||||||
return isOffline; | ||||||
} | ||||||
|
||||||
/** | ||||||
* @returns {String} | ||||||
*/ | ||||||
|
@@ -75,15 +103,32 @@ function getCurrentUserEmail() { | |||||
/** | ||||||
* @returns {Boolean} | ||||||
*/ | ||||||
function isReady() { | ||||||
return networkReady; | ||||||
function hasReadRequiredDataFromStorage() { | ||||||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
return hasReadRequiredData; | ||||||
} | ||||||
|
||||||
/** | ||||||
* @param {Boolean} value | ||||||
*/ | ||||||
function setIsAuthenticating(value) { | ||||||
authenticating = value; | ||||||
} | ||||||
|
||||||
/** | ||||||
* @returns {Boolean} | ||||||
*/ | ||||||
function isAuthenticating() { | ||||||
return authenticating; | ||||||
} | ||||||
|
||||||
export { | ||||||
getAuthToken, | ||||||
setAuthToken, | ||||||
getCredentials, | ||||||
getCurrentUserEmail, | ||||||
isReady, | ||||||
setIsReady, | ||||||
hasReadRequiredDataFromStorage, | ||||||
setHasReadRequiredDataFromStorage, | ||||||
setIsAuthenticating, | ||||||
isAuthenticating, | ||||||
getIsOffline, | ||||||
}; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,96 @@ | ||||||
import _ from 'underscore'; | ||||||
import Onyx from 'react-native-onyx'; | ||||||
import * as PersistedRequests from '../actions/PersistedRequests'; | ||||||
import * as NetworkStore from './NetworkStore'; | ||||||
import * as NetworkEvents from './NetworkEvents'; | ||||||
import CONST from '../../CONST'; | ||||||
import ONYXKEYS from '../../ONYXKEYS'; | ||||||
import * as ActiveClientManager from '../ActiveClientManager'; | ||||||
import processRequest from './processRequest'; | ||||||
|
||||||
let persistedRequestsQueueRunning = false; | ||||||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
/** | ||||||
* This method will get any persisted requests and fire them off in parallel to retry them. | ||||||
* If we get any jsonCode besides 407 the request is a success. It doesn't make sense to | ||||||
* continually retry things that have returned a response. However, we can retry any requests | ||||||
* with known networking errors like "Failed to fetch". | ||||||
* | ||||||
* @returns {Promise} | ||||||
*/ | ||||||
function process() { | ||||||
const persistedRequests = PersistedRequests.getAll(); | ||||||
|
||||||
// This sanity check is also a recursion exit point | ||||||
if (NetworkStore.getIsOffline() || _.isEmpty(persistedRequests)) { | ||||||
return Promise.resolve(); | ||||||
} | ||||||
|
||||||
const tasks = _.map(persistedRequests, request => processRequest(request) | ||||||
.then((response) => { | ||||||
if (response.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED) { | ||||||
NetworkEvents.getLogger().info('Persisted optimistic request needs authentication'); | ||||||
} else { | ||||||
NetworkEvents.getLogger().info('Persisted optimistic request returned a valid jsonCode. Not retrying.'); | ||||||
} | ||||||
NetworkEvents.onResponse(request, response); | ||||||
PersistedRequests.remove(request); | ||||||
}) | ||||||
.catch((error) => { | ||||||
// If we are catching a known network error like "Failed to fetch" allow this request to be retried if we have retries left | ||||||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if (error.message === CONST.ERROR.FAILED_TO_FETCH) { | ||||||
const retryCount = PersistedRequests.incrementRetries(request); | ||||||
NetworkEvents.getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message}); | ||||||
if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) { | ||||||
NetworkEvents.getLogger().info('Request failed too many times removing from storage', false, {retryCount, command: request.command, error: error.message}); | ||||||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
PersistedRequests.remove(request); | ||||||
} | ||||||
} else if (error.name === CONST.ERROR.REQUEST_CANCELLED) { | ||||||
NetworkEvents.getLogger().info('Persisted request was cancelled. Not retrying.'); | ||||||
NetworkEvents.onError(request); | ||||||
PersistedRequests.remove(request); | ||||||
} else { | ||||||
NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error while retrying persisted request. Not retrying.`, { | ||||||
command: request.command, | ||||||
error: error.message, | ||||||
}); | ||||||
PersistedRequests.remove(request); | ||||||
} | ||||||
})); | ||||||
|
||||||
// Do a recursive call in case the queue is not empty after processing the current batch | ||||||
return Promise.all(tasks) | ||||||
.then(process); | ||||||
} | ||||||
|
||||||
function flush() { | ||||||
if (persistedRequestsQueueRunning) { | ||||||
return; | ||||||
} | ||||||
|
||||||
// ONYXKEYS.PERSISTED_REQUESTS is shared across clients, thus every client/tab will have a copy | ||||||
// It is very important to only process the queue from leader client otherwise requests will be duplicated. | ||||||
if (!ActiveClientManager.isClientTheLeader()) { | ||||||
return; | ||||||
} | ||||||
|
||||||
persistedRequestsQueueRunning = true; | ||||||
|
||||||
// Ensure persistedRequests are read from storage before proceeding with the queue | ||||||
const connectionId = Onyx.connect({ | ||||||
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. Sorry, it's probably just a me problem, but I'm confused how this is working and have a few questions:
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. You're right. It's a hack that ensures we don't start processing the queue until the requests have been read from storage. This mostly only matters on app init so we could probably improve this further (as we are subscribing in
Main usage here and runs when we go from offline to online App/src/libs/Network/PersistedRequestsQueue.js Lines 90 to 91 in f4c1cb2
|
||||||
key: ONYXKEYS.PERSISTED_REQUESTS, | ||||||
callback: () => { | ||||||
Onyx.disconnect(connectionId); | ||||||
process() | ||||||
.finally(() => persistedRequestsQueueRunning = false); | ||||||
}, | ||||||
}); | ||||||
} | ||||||
|
||||||
// Flush the queue when the connection resumes | ||||||
NetworkEvents.onConnectivityResumed(flush); | ||||||
|
||||||
export { | ||||||
// eslint-disable-next-line import/prefer-default-export | ||||||
flush, | ||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
export default class RetryCounter { | ||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
constructor() { | ||
this.retryMap = new Map(); | ||
} | ||
|
||
clear() { | ||
this.retryMap.clear(); | ||
} | ||
|
||
/** | ||
* @param {Object} request | ||
* @returns {Number} retry count | ||
*/ | ||
incrementRetries(request) { | ||
const current = this.retryMap.get(request) || 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. TIL that objects can be keys for Maps ... but that seems strange / like it might have unexpected side-effects (honestly not sure). There very well might be no problem here, but maybe it would be clearer to use a 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. That makes sense to me, but at the same time I'm going to leave this since the code already existed and if it's confusing at least it's abstracted in this class for now :D |
||
const next = current + 1; | ||
this.retryMap.set(request, next); | ||
return next; | ||
} | ||
|
||
/** | ||
* @param {Object} request | ||
*/ | ||
remove(request) { | ||
this.retryMap.delete(request); | ||
} | ||
} |
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: I think we can use
NetworkStore.isAuthenticating()
andNetworkStore.setAuthenticating(value)
for this boolean value, no? https://airbnb.io/javascript/#accessors--boolean-prefixThere 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.
Maybe - though unsure if we follow that guidance strictly. I had it like that but changed it as I couldn't think of a way to do that without changing the variable name here...
App/src/libs/Network/NetworkStore.js
Line 11 in c39f2cf
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 wouldn't see a problem renaming it to
authenticating
since it's only used as a private variable and theNetworkStore
is a small file, but that's just my opinion :)