-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Unpausing the queue when we receive Pusher updates while we're still loading the App #40169
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
Changes from all commits
192aa7f
16e6a3f
15a1fcd
f9f9a09
8a63405
7b48876
1f3829c
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 |
|---|---|---|
|
|
@@ -41,11 +41,18 @@ export default () => { | |
| Onyx.connect({ | ||
| key: ONYXKEYS.ONYX_UPDATES_FROM_SERVER, | ||
| callback: (value) => { | ||
| // When the OpenApp command hasn't finished yet, we should not process any updates. | ||
| if (!value || isLoadingApp) { | ||
| if (isLoadingApp) { | ||
| console.debug(`[OnyxUpdateManager] Ignoring Onyx updates while OpenApp hans't finished yet.`); | ||
| } | ||
| // When there's no value, there's nothing to process, so let's return early. | ||
| if (!value) { | ||
| return; | ||
| } | ||
| // If isLoadingApp is positive it means that OpenApp command hasn't finished yet, and in that case | ||
| // we don't have base state of the app (reports, policies, etc) setup. If we apply this update, | ||
| // we'll only have them overriten by the openApp response. So let's skip it and return. | ||
| if (isLoadingApp) { | ||
| // When ONYX_UPDATES_FROM_SERVER is set, we pause the queue. Let's unpause | ||
| // it so the app is not stuck forever without processing requests. | ||
| SequentialQueue.unpause(); | ||
|
Contributor
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. Please add a comment to explain why this needs to be here. Also, can you update the comment on line 44 above so that it also explains why updates shouldn't be processed in that situation? |
||
| console.debug(`[OnyxUpdateManager] Ignoring Onyx updates while OpenApp hans't finished yet.`); | ||
| return; | ||
| } | ||
| // This key is shared across clients, thus every client/tab will have a copy and try to execute this method. | ||
|
|
@@ -80,11 +87,6 @@ export default () => { | |
| // fully migrating to the reliable updates mode. | ||
| // 2. This client already has the reliable updates mode enabled, but it's missing some updates and it | ||
| // needs to fetch those. | ||
| // | ||
| // For both of those, we need to pause the sequential queue. This is important so that the updates are | ||
| // applied in their correct and specific order. If this queue was not paused, then there would be a lot of | ||
| // onyx data being applied while we are fetching the missing updates and that would put them all out of order. | ||
| SequentialQueue.pause(); | ||
| let canUnpauseQueuePromise; | ||
|
|
||
| // The flow below is setting the promise to a reconnect app to address flow (1) explained above. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,6 +119,9 @@ function apply({lastUpdateID, type, request, response, updates}: OnyxUpdatesFrom | |
| * @param [updateParams.updates] Exists if updateParams.type === 'pusher' | ||
| */ | ||
| function saveUpdateInformation(updateParams: OnyxUpdatesFromServer) { | ||
| // Unpause the Sequential queue so we go back to processing requests to guarantee we don't make the app unusable | ||
| SequentialQueue.unpause(); | ||
|
Contributor
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. The comment says to pause the queue, but this is unpausing the queue 😓
Contributor
Author
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. Wrong comment, should be written unpause. |
||
|
|
||
| // Always use set() here so that the updateParams are never merged and always unique to the request that came in | ||
| Onyx.set(ONYXKEYS.ONYX_UPDATES_FROM_SERVER, updateParams); | ||
| } | ||
|
|
@@ -148,9 +151,6 @@ function applyOnyxUpdatesReliably(updates: OnyxUpdatesFromServer) { | |
| apply(updates); | ||
| return; | ||
| } | ||
|
|
||
| // If we reached this point, we need to pause the queue while we prepare to fetch older OnyxUpdates. | ||
| SequentialQueue.pause(); | ||
| saveUpdateInformation(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.
That assumes that the data is coming in OpenApp, but it might not.