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.

Sign up for GitHub

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

Jump to bottom

Fix concurrent writes in the update operation #384

Merged
merged 5 commits into from
Oct 6, 2023
Merged

Fix concurrent writes in the update operation #384

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a941bd8
Fix concurrent writes in the update operation
ospfranco Oct 4, 2023
e26d5b5
Lint
ospfranco Oct 4, 2023
8167a91
Rewrite innerUpdate function for clarity
ospfranco Oct 4, 2023
944bfac
Do not export onyx get operation
ospfranco Oct 5, 2023
24971b6
Make sure clear operation on onyx.update executes first
ospfranco Oct 6, 2023
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
58 changes: 32 additions & 26 deletions lib/Onyx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1392,6 +1392,36 @@ function mergeCollection(collectionKey, collection) {
});
}

function innerUpdate(data) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add JSDoc

if (data.length === 0) {
return Promise.resolve();
}

const {onyxMethod, key, value} = data.shift();
let promise = Promise.resolve();
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;
}
Comment on lines +1408 to +1426
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


return promise.then(() => innerUpdate(data));
}

/**
* Insert API responses and lifecycle data into Onyx
*
Expand All @@ -1414,32 +1444,7 @@ function update(data) {
}
});

const promises = [];
let clearPromise = Promise.resolve();

_.each(data, ({onyxMethod, key, value}) => {
switch (onyxMethod) {
case METHOD.SET:
promises.push(() => set(key, value));
break;
case METHOD.MERGE:
promises.push(() => merge(key, value));
break;
case METHOD.MERGE_COLLECTION:
promises.push(() => mergeCollection(key, value));
break;
case METHOD.MULTI_SET:
promises.push(() => multiSet(value));
break;
case METHOD.CLEAR:
clearPromise = clear();
break;
default:
break;
}
});

return clearPromise.then(() => Promise.all(_.map(promises, p => p())));
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ospfranco ospfranco Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return innerUpdate(data);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

/**
Expand Down Expand Up @@ -1534,6 +1539,7 @@ const Onyx = {
connect,
disconnect,
set,
get,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

multiSet,
merge,
mergeCollection,
Expand Down
43 changes: 40 additions & 3 deletions tests/unit/onyxTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,9 +899,9 @@ describe('Onyx', () => {
{onyxMethod: Onyx.METHOD.MERGE_COLLECTION, key: ONYX_KEYS.COLLECTION.TEST_UPDATE, value: {[itemKey]: {a: 'a'}}},
])
.then(() => {
expect(collectionCallback).toHaveBeenNthCalledWith(1, {[itemKey]: {a: 'a'}});
expect(testCallback).toHaveBeenNthCalledWith(1, 'taco', ONYX_KEYS.TEST_KEY);
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Onyx.disconnect(connectionIDs);
});
});
Expand Down Expand Up @@ -960,4 +960,41 @@ describe('Onyx', () => {
});
});
});

it('should write data in order', () => {
const key = `${ONYX_KEYS.TEST_KEY}123`;
connectionID = Onyx.connect({
key,
initWithStoredValues: false,
callback: () => null,
});

return waitForPromisesToResolve()
.then(() => {
// Given the initial Onyx state: {test: true, otherTest: {test1: 'test1'}}
Onyx.set(key, true);
return waitForPromisesToResolve();
})
.then(() => Onyx.update([
{
onyxMethod: 'set',
key,
value: 'one',
},
{
onyxMethod: 'merge',
key,
value: 'two',
},
{
onyxMethod: 'set',
key,
value: 'three',
},
]))
.then(() => Onyx.get(key))
.then((value) => {
expect(value).toBe('three');
});
});
});
Loading