Revert "Revert "Debounce dispatched state diffs"" #3579
Conversation
This reverts commit b8d911b.
Tracks pending store updates to delay async thunk responses until state changes are commited to the UI proxied store. This fixes cases where thunks resolve too early i.e. before the aggregated updates have been sent to the UI. This relies on the subscribed callback from webext-redux for diffing to have executed before the dispatched thunk has finished executing in the background script.
This fixes an issue that caused additional delay on thunks that only return a value, but, still rely on other async operations.
hyphenized
force-pushed
the
wait-aggregation
branch
from
e4f9295
to
5b169d7
Compare
There was a problem hiding this comment.
Solution feels strong. Left a couple of notes on how we might tighten it up just a touch. I wish we could go ahead and do our own fork of webext-redux already, but I just remembered when I tried to do this I had to face down some dependency stuff because the package hasn't seen a lot of updates the last couple of years… For another time.
background/main.ts
Outdated
| pendingUpdates.forEach((resolve) => resolve()) | ||
|
|
||
| // Clear all currently tracked updates | ||
| while (pendingUpdates.length) { | ||
| pendingUpdates.pop() | ||
| } |
There was a problem hiding this comment.
If I'm not mistaken and the below remark about not needing an array doesn't apply, I think this can be rewritten as:
| pendingUpdates.forEach((resolve) => resolve()) | |
| // Clear all currently tracked updates | |
| while (pendingUpdates.length) { | |
| pendingUpdates.pop() | |
| } | |
| pendingUpdates | |
| .splice(0) | |
| .forEach((resolve) => resolve()) |
background/main.ts
Outdated
| // by this time, all pending updates should've been tracked. | ||
| // since we're dealing with a thunk, we need to wait for | ||
| // the store to be updated | ||
| if (isThunk) await storeUpdateLock |
There was a problem hiding this comment.
Do we suffer if we do this for all dispatches?
There was a problem hiding this comment.
Good point! We don't because this is only relevant to dispatch responses
background/main.ts
Outdated
| return diffWrapper === undefined ? [] : [diffWrapper] | ||
| diffStrategy: (oldObj, newObj, forceUpdate) => { | ||
| storeUpdateLock = new Promise((resolve) => { | ||
| pendingUpdates.push(resolve) |
There was a problem hiding this comment.
Do we need this to be an array? It feels like storeUpdateLock can be one Promise<void> | undefined with one associated unlocker function:
// let storeUpdateLock: Promise<void> | undefined
// let unlockStoreUpdate: (_: void) => void | undefined
if (storeUpdateLock === undefined) {
// Set read lock so others can block on it.
storeUpdateLock = new Promise((resolve) => unlockStoreUpdate = resolve)
}
queueUpdate(oldObj, newObj, forceUpdate)
// Return no diffs.......
return []Then queueUpdate, when it's called debounced, can just resolve (and clear) the one promise and its resolver, and everyone who has blocked in the meantime will be awaiting that one promise. I might not be playing it all out correctly in my brain though, I'm not at 100%.
There was a problem hiding this comment.
I think this could work but it might cause unexpected behaviour if a thunk performs a dispatch and takes longer than 30ms to complete 🤔
There was a problem hiding this comment.
I'm wondering if that's functionally different than the current version. In both cases, storeUpdateLock's value becomes relevant once the thunk is resolved, and in both cases the storeUpdateLock promise is resolved as soon as the diff starts running. My read is that means the array and non-array versions would be identical in behavior, but these are def complicated interactions to reason about so I could totally be wrong.
There was a problem hiding this comment.
Ahhh I maybe see what you're saying. I think this change from array of promises to single promise would be functionally identical, but the Promise.all approach in the previous comment would not be, because using Promise.all reads the lock before the thunk has completed, while the behavior we want here is to let the thunk finish, then wait until the latest debounced state update is pushed before responding with the thunk outcome.
There was a problem hiding this comment.
Yup they are identical, refactored the array out in favour of a single lock (forgot to submit these comments 🤦♂️ )
|
None of the above seem blocking, so I'm going to give this a test whirl and plan to merge it for next week's release whether or not we get a chance to work on them. |
Only the last dispatched update from the thunk is needed to confirm that the store has been synced on the UI side and it's now safe to complete the thunk.
Fixes a bunch of errors caused from re-rendering accounts with same-symbol assets.
There was a problem hiding this comment.
Looks like when we force update after the thunk is done, during dApp signing, we can try to push something through a disconnected port (since the popover has closed out). Shouldn't be a huge issue in practice, but might be nice to catch it and log it at a lower level than error.
Not blocking, wallet seems to be behaving correctly across the board, tried dApps and swaps as well as settings toggles and the mentioned send/reject flow. Let's roll it!
| if (!storeUpdateLock) { | ||
| storeUpdateLock = new Promise((resolve) => { | ||
| releaseLock = () => { | ||
| resolve() | ||
| storeUpdateLock = null | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Golfing at this point, but:
| if (!storeUpdateLock) { | |
| storeUpdateLock = new Promise((resolve) => { | |
| releaseLock = () => { | |
| resolve() | |
| storeUpdateLock = null | |
| } | |
| }) | |
| } | |
| storeUpdateLock ??= new Promise((resolve) => { | |
| releaseLock = () => { | |
| resolve() | |
| storeUpdateLock = null | |
| } | |
| } |
## Highlights - Further performance improvements. - Groundwork to rename backend dApp connection things (for the provider) to Taho naming. ## What's Changed * v0.46.1 by @Shadowfiend in #3598 * Updates to release checklist: Removed Mintkudos test by @andreachapman in #3600 * Revert "Revert "Debounce dispatched state diffs"" by @hyphenized in #3579 * Underbridge: Drop updated dApp connections feature flag, further Taho renames by @Shadowfiend in #3595 **Full Changelog**: v0.46.1...v0.47.0 Latest build: [extension-builds-3601](https://github.com/tahowallet/extension/suites/15160003504/artifacts/866327987) (as of Wed, 16 Aug 2023 19:09:51 GMT).
Reimplements debounced/aggregated diffs dispatched on store updates and fixes awaiting of dispatched thunks from the UI.
Added some logic to track pending store updates on the dispatch responder. This is done so that async thunk responses are postponed until state changes are committed to the UI's proxied store. This fixes cases where thunks cause unexpected behaviour by resolving too early, before the aggregated updates have been sent to the UI.
This relies on the subscribed callback from webext-redux for diffing to have executed before the dispatched thunk has finished executing in the background script.
To Test
A consistent way to reproduce this issue is to attempt to initiate a transfer through the Send Asset page and then reject it.
Internally the route is meant to change once the thunk completes. Thus, if we change the route before the transaction state has been cleared by the thunk completion, we are incorrectly sent back to the sign transaction screen. This PR also addresses similar issues.
Latest build: extension-builds-3579 (as of Tue, 15 Aug 2023 19:39:44 GMT).