Fix concurrent writes in the update operation #384

ospfranco · 2023-10-04T11:36:30Z

Details

The update operation was written in such manner that it fired all database operations at once. Some of the operations internally rely on other promises, meaning they will get re-scheduled on another tick. This causes unpredictable behavior as you cannot guarantee the order of the operations. The solution is clear and is the only one, all DB writes must respect the order of the operations.

The incorrect ordering of the operations being committed to the DB causes other random behavior around the app as well. Linked the two issues.

Related Issues

Expensify/App#28737
Expensify/App#28584

Automated Tests

Added one more test that makes sure the correct data is persisted. It failed with the previous implementation, now it passes.

Linked PRs

ospfranco · 2023-10-04T14:19:46Z

tests/unit/onyxTest.js

- expect(otherTestCallback).toHaveBeenNthCalledWith(1, 'pizza', ONYX_KEYS.OTHER_TEST);
+ expect(collectionCallback).toHaveBeenNthCalledWith(2, {[itemKey]: {a: 'a'}});
+ expect(testCallback).toHaveBeenNthCalledWith(2, 'taco', ONYX_KEYS.TEST_KEY);
+ expect(otherTestCallback).toHaveBeenNthCalledWith(2, 'pizza', ONYX_KEYS.OTHER_TEST);


This previously was receiving the write updates before the hydration callback, but now that the write operations take a little longer the order is correct.

ospfranco · 2023-10-04T14:21:06Z

lib/Onyx.js

- });
-
- return clearPromise.then(() => Promise.all(_.map(promises, p => p())));
+ return innerUpdate(data);


As a data point, I tried the proposed solution by Marc which would try to resolve earlier while keeping the writes async, this however is not correct and broke a bunch of other tests.

tgolen

Has there been any testing done in App to see what kind of performance impact this will have? If so, can you please attach it to this PR?

tgolen · 2023-10-04T16:43:10Z

lib/Onyx.js

@@ -1534,6 +1539,7 @@ const Onyx = {
 connect,
 disconnect,
 set,
+ get,


It was pretty import to avoid exporting this so that people don't get lazy in App and start using it all over the place (which encourages a lot of chaining calls). It looks like you're exporting it for use in the tests and I would like to propose these alternatives:

Inside the tests use Onyx.connect() to get the value (this is the preferred method)

Rename this export to something like onlyToBeUsedInTestCode_get

From what I saw Onyx.connect has caching implemented, which might not be what is actually persisted on the DB. I was about to directly query the DB which is at the end of the day what really matters. I think this get might also use caches right? if so, checking the DB should be the correct path.

I like the idea of querying the DB directly if that's possible.

ospfranco · 2023-10-04T16:48:14Z

No, I haven't tested the app. I can try to get some metrics but I already asked in slack if there is any particular call to update that is heavy.

tgolen · 2023-10-04T16:50:58Z

The OpenApp and ReconnectApp API commands would be the best place to start. Those are the ones receiving the largest calls to Onyx.update().

ospfranco · 2023-10-05T06:36:32Z

I added some react-native-performance measures on app start, I cannot tell which one exactly is OpenApp but they are all basically negible. Writing to DB should not be the bottleneck, and even then, correctness is not negotiable.

tgolen · 2023-10-05T15:16:26Z

OK, I just created a large account for testing things like this, so let me give it a try with this today.

tgolen

I tested it in App with a large account and I couldn't notice any performance impact, so I think these are good changes. I just noticed this one thing.

tgolen · 2023-10-05T15:33:03Z

lib/Onyx.js

@@ -1392,6 +1392,36 @@ function mergeCollection(collectionKey, collection) {
 });
 }

+function innerUpdate(data) {


Please add JSDoc

marcaaron · 2023-10-05T20:58:22Z

Doing some testing now...

marcaaron · 2023-10-05T21:59:04Z

Alright well - it is defying my expectations a bit, but when timing the calls to Onyx.update() they are running way faster synchronously 😅

Sign in trials

With sync updates:

1

Timing:development.new.expensify.flushQueue 458
Timing.js:49 Timing:development.new.expensify.flushQueue 0
Timing.js:49 Timing:development.new.expensify.flushQueue 407

2

Timing:development.new.expensify.flushQueue 468
Timing.js:49 Timing:development.new.expensify.flushQueue 1
Timing.js:49 Timing:development.new.expensify.flushQueue 191

3

Timing:development.new.expensify.flushQueue 311
Timing.js:49 Timing:development.new.expensify.flushQueue 0
Timing.js:49 Timing:development.new.expensify.flushQueue 226

Without sync updates (but on latest react-native-onyx):

1

Timing:development.new.expensify.flushQueue 488
Timing.js:49 Timing:development.new.expensify.flushQueue 4147
Timing.js:49 Timing:development.new.expensify.flushQueue 962

2

Timing:development.new.expensify.flushQueue 420
Timing.js:49 Timing:development.new.expensify.flushQueue 1126
Timing.js:49 Timing:development.new.expensify.flushQueue 170

3

Timing:development.new.expensify.flushQueue 464
Timing.js:49 Timing:development.new.expensify.flushQueue 4729
Timing.js:49 Timing:development.new.expensify.flushQueue 976

This was a web-only test. Haven't tested on iOS or Android - but looks promising.

marcaaron · 2023-10-05T22:01:19Z

lib/Onyx.js

- }
- });
-
- return clearPromise.then(() => Promise.all(_.map(promises, p => p())));


I see we are undoing the previous logic to move the clear() to the front of the update queue. Does running the updates sync solve the issue that caused us to make that change?

ah, probably not. I'll take care of it.

I'm not sure making sure clear runs first is a desired behavior but I guess now it is too late to change it, other parts of the system might rely on it. I added sorting to the list to make sure clear operations get executed first.

Yeah, I agree it is a bit of a weird hack. But changing it could have unexpected consequences. Thanks for updating.

marcaaron

🚀

tgolen · 2023-10-10T15:04:54Z

lib/Onyx.js

+ switch (onyxMethod) {
+ case METHOD.SET:
+ promise = set(key, value);
+ break;
+ case METHOD.MERGE:
+ promise = merge(key, value);
+ break;
+ case METHOD.MERGE_COLLECTION:
+ promise = mergeCollection(key, value);
+ break;
+ case METHOD.MULTI_SET:
+ promise = multiSet(value);
+ break;
+ case METHOD.CLEAR:
+ promise = clear();
+ break;
+ default:
+ break;
+ }


It looks like this change possibly led to some regressions spotted in this PR and discussed in this Slack thread that updated the Onyx version

Fix concurrent writes in the update operation

a941bd8

ospfranco requested a review from a team as a code owner October 4, 2023 11:36

melvin-bot bot requested review from deetergp and removed request for a team October 4, 2023 11:37

Lint

e26d5b5

ospfranco commented Oct 4, 2023

View reviewed changes

Rewrite innerUpdate function for clarity

8167a91

mountiny requested review from tgolen and marcaaron October 4, 2023 14:44

tgolen requested changes Oct 4, 2023

View reviewed changes

Do not export onyx get operation

944bfac

ospfranco requested a review from tgolen October 5, 2023 06:37

tgolen requested changes Oct 5, 2023

View reviewed changes

marcaaron reviewed Oct 5, 2023

View reviewed changes

marcaaron assigned ospfranco Oct 5, 2023

Make sure clear operation on onyx.update executes first

24971b6

ospfranco requested review from marcaaron and tgolen October 6, 2023 06:03

tgolen approved these changes Oct 6, 2023

View reviewed changes

jjcoffee mentioned this pull request Oct 6, 2023

[$1000] Web - Onyx: out-of-order updates Expensify/App#28737

Closed

6 tasks

deetergp approved these changes Oct 6, 2023

View reviewed changes

marcaaron approved these changes Oct 6, 2023

View reviewed changes

marcaaron merged commit 01e2bab into Expensify:main Oct 6, 2023
3 checks passed

ospfranco mentioned this pull request Oct 10, 2023

[NoQA] Bump onyx - Fix out of order updates Expensify/App#29169

Merged

59 tasks

tgolen reviewed Oct 10, 2023

View reviewed changes

tgolen mentioned this pull request Oct 10, 2023

Revert "Fix concurrent writes in the update operation" #397

Merged

hungvu193 mentioned this pull request Oct 25, 2023

[PAID] [$500] Web - Signing show empty content when pressing go back from another tab. Expensify/App#30288

Closed

6 tasks

tgolen mentioned this pull request Nov 8, 2023

[HOLD for payment 2023-11-29] [$1000] Removed reactions on offline mode re appear on the receiver account and there is no way to remove them Expensify/App#23679

Closed

6 tasks

hannojg mentioned this pull request Feb 26, 2024

Asset loading margelo/react-native-filament#13

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix concurrent writes in the update operation #384

Fix concurrent writes in the update operation #384

ospfranco commented Oct 4, 2023

ospfranco Oct 4, 2023

ospfranco Oct 4, 2023

tgolen left a comment

tgolen Oct 4, 2023

ospfranco Oct 4, 2023

tgolen Oct 4, 2023

ospfranco Oct 5, 2023

ospfranco commented Oct 4, 2023

tgolen commented Oct 4, 2023

ospfranco commented Oct 5, 2023

tgolen commented Oct 5, 2023

tgolen left a comment

tgolen Oct 5, 2023

marcaaron commented Oct 5, 2023

marcaaron commented Oct 5, 2023

marcaaron Oct 5, 2023

ospfranco Oct 6, 2023

ospfranco Oct 6, 2023 •

edited

Loading

marcaaron Oct 6, 2023

marcaaron left a comment

tgolen Oct 10, 2023

Fix concurrent writes in the update operation #384

Fix concurrent writes in the update operation #384

Conversation

ospfranco commented Oct 4, 2023

Details

Related Issues

Automated Tests

Linked PRs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ospfranco commented Oct 4, 2023

tgolen commented Oct 4, 2023

ospfranco commented Oct 5, 2023

tgolen commented Oct 5, 2023

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Oct 5, 2023

marcaaron commented Oct 5, 2023

Sign in trials

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ospfranco Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ospfranco Oct 6, 2023 •

edited

Loading