[HOLD] Remove macro/micro tasks during subscriber update#724
[HOLD] Remove macro/micro tasks during subscriber update#724VickyStash wants to merge 24 commits intoExpensify:mainfrom
Conversation
# Conflicts: # lib/Onyx.ts # lib/OnyxMerge/index.native.ts
# Conflicts: # lib/Onyx.ts
|
LGTM |
VickyStash
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6318f5ef65
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (lastConnectionCallbackData.has(mapping.subscriptionID) && valueToPass === lastValue) { | ||
| // If the subscriber was already notified (e.g. by a synchronous keyChanged call), | ||
| // skip the initial data delivery to prevent duplicate callbacks. | ||
| if (lastConnectionCallbackData.has(mapping.subscriptionID)) { |
There was a problem hiding this comment.
Keep sendDataToConnection() from returning solely on lastConnectionCallbackData.has(...); this now drops the initial hydration callback whenever any synchronous keyChanged/keysChanged ran first. In the common race where Onyx.connect() is followed by an immediate Onyx.set() in the same tick, the subscription gets marked as "already notified" and the later storage-backed init payload is skipped even if it contains additional data (especially for collection subscribers), leaving subscribers with a partial state until a future update arrives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
In this case, by the time the hydration promise resolves, it would only have equal or older data, making the skip correct.
There was a problem hiding this comment.
@VickyStash I asked Claude about this comment and according to it it's valid, here's a unit test it designed for me
diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts
index 64caec5..94aff05 100644
--- a/tests/unit/onyxTest.ts
+++ b/tests/unit/onyxTest.ts
@@ -140,6 +140,51 @@ describe('Onyx', () => {
});
});
+ it('should deliver full collection data when connect() is followed by immediate set() of a single member in the same tick', () => {
+ const mockCallback = jest.fn();
+ const collectionKey = ONYX_KEYS.COLLECTION.TEST_CONNECT_COLLECTION;
+
+ // Write collection members directly to storage, bypassing Onyx cache values.
+ // This simulates data that exists in persistent storage but hasn't been loaded into cache yet
+ // (e.g. from a previous session).
+ return StorageMock.setItem(`${collectionKey}1`, {ID: 1, value: 'one'})
+ .then(() => StorageMock.setItem(`${collectionKey}2`, {ID: 2, value: 'two'}))
+ .then(() => StorageMock.setItem(`${collectionKey}3`, {ID: 3, value: 'three'}))
+ .then(() => {
+ // Register the keys in Onyx's key cache so getAllKeys() can discover them.
+ // We intentionally do NOT add values to the cache — only keys — to simulate
+ // data that is known to exist but whose values haven't been hydrated yet.
+ cache.addKey(`${collectionKey}1`);
+ cache.addKey(`${collectionKey}2`);
+ cache.addKey(`${collectionKey}3`);
+
+ // Connect to the collection — this starts an async storage read (microtask)
+ connection = Onyx.connect({
+ key: collectionKey,
+ waitForCollectionCallback: true,
+ callback: mockCallback,
+ });
+
+ // Immediately set a single member in the same synchronous tick.
+ // This triggers synchronous keyChanged() which calls the subscriber with a partial
+ // collection (just _1 from the cache). This sets lastConnectionCallbackData for this
+ // subscriber. The async hydration from subscribeToKey should still deliver the full
+ // collection afterwards, since the data is different.
+ Onyx.set(`${collectionKey}1`, {ID: 1, value: 'updated'});
+
+ // Wait for all async operations (storage reads from subscribeToKey) to complete
+ return waitForPromisesToResolve();
+ })
+ .then(() => {
+ // The subscriber's final state should contain ALL collection members,
+ // including _2 and _3 that were only in storage (not cache) at the time of the synchronous keyChanged call.
+ const lastCall = mockCallback.mock.calls[mockCallback.mock.calls.length - 1][0];
+ expect(lastCall).toHaveProperty(`${collectionKey}1`);
+ expect(lastCall).toHaveProperty(`${collectionKey}2`);
+ expect(lastCall).toHaveProperty(`${collectionKey}3`);
+ });
+ });
+
it('should merge an object with another object', () => {
let testKeyValue: unknown;
Could you validate if it makes sense?
There was a problem hiding this comment.
@VickyStash I asked Claude about this comment and according to it it's valid, here's a unit test it designed for me
Wow, nice catch! Claude have gave me totally different results when I asked it back to then.
I agree, that's truly can be a problem, I'm looking into how to handle it correctly.
Adjusted jest test, if anyone is interested
test('connect() followed by immediate set() should still deliver full collection from storage', async () => {
const mockCallback = jest.fn();
// ===== Session 1 =====
// Data is written to persistent storage (simulates a previous app session).
await StorageMock.setItem(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {id: 1, title: 'Test One'});
await StorageMock.setItem(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`, {id: 2, title: 'Test Two'});
await StorageMock.setItem(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`, {id: 3, title: 'Test Three'});
// ===== Session 2 =====
// App restarts. Onyx.init() calls getAllKeys() which populates storageKeys
// with all 3 keys, but their values are NOT read into cache yet.
Onyx.init({keys: ONYX_KEYS});
// A component connects to the collection (starts async hydration via multiGet).
Onyx.connect({
key: ONYX_KEYS.COLLECTION.TEST_KEY,
waitForCollectionCallback: true,
callback: mockCallback,
});
Onyx.set(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {id: 1, title: 'Updated Test One'});
await waitForPromisesToResolve();
// The subscriber should eventually receive ALL collection members.
// The async hydration reads test_2 and test_3 from storage.
const lastCall = mockCallback.mock.calls[mockCallback.mock.calls.length - 1][0];
expect(lastCall).toHaveProperty(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`);
expect(lastCall).toHaveProperty(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`);
expect(lastCall).toHaveProperty(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`);
// Verify the updated value is present (not stale)
expect(lastCall[`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]).toEqual({id: 1, title: 'Updated Test One'});
});
|
I asked @VickyStash to hold this PR because of this plan to handle the canBeMissing situation -> Expensify/App#80095 (comment) |
VickyStash
changed the title
|
Not holded anymore! |
|
@Krishna2323 can you please test this pr with App? |
|
Will be on it in an hour or two. |
|
Reviewing... |
|
@Krishna2323 Please, take another look! |
|
@VickyStash could you please check this comment? Sorry about the wrong suggestion. 🙏🏻 |
|
I’ll try to finish the review by tomorrow — I had to step out for a doctor’s appointment. Thanks for the patience. |
| if (isSubscribedToCollectionKey) { | ||
| if (subscriber.waitForCollectionCallback) { | ||
| lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection); | ||
| subscriber.callback(cachedCollection, subscriber.key, partialCollection); | ||
| continue; | ||
| } | ||
|
|
||
| // If they are not using waitForCollectionCallback then we notify the subscriber with | ||
| // the new merged data but only for any keys in the partial collection. | ||
| const dataKeys = Object.keys(partialCollection ?? {}); | ||
| for (const dataKey of dataKeys) { | ||
| if (deepEqual(cachedCollection[dataKey], previousCollection[dataKey])) { | ||
| continue; | ||
| } | ||
|
|
||
| subscriber.callback(cachedCollection[dataKey], dataKey); | ||
| } | ||
| lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts
index dd08e6b..06d17c1 100644
--- a/lib/OnyxUtils.ts
+++ b/lib/OnyxUtils.ts
@@ -686,8 +686,9 @@ function keysChanged<TKey extends CollectionKeyBase>(
// If they are subscribed to the collection key and using waitForCollectionCallback then we'll
// send the whole cached collection.
if (isSubscribedToCollectionKey) {
+ lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection);
+
if (subscriber.waitForCollectionCallback) {
- lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection);
subscriber.callback(cachedCollection, subscriber.key, partialCollection);
continue;
}
@@ -702,7 +703,6 @@ function keysChanged<TKey extends CollectionKeyBase>(
subscriber.callback(cachedCollection[dataKey], dataKey);
}
- lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection);
continue;
}
Maybe we can just do lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection); right inside if (isSubscribedToCollectionKey) { since it will be executed in all situations inside this condition?
| if (lastConnectionCallbackData.has(mapping.subscriptionID) && valueToPass === lastValue) { | ||
| // If the subscriber was already notified (e.g. by a synchronous keyChanged call), | ||
| // skip the initial data delivery to prevent duplicate callbacks. | ||
| if (lastConnectionCallbackData.has(mapping.subscriptionID)) { |
There was a problem hiding this comment.
@VickyStash I asked Claude about this comment and according to it it's valid, here's a unit test it designed for me
diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts
index 64caec5..94aff05 100644
--- a/tests/unit/onyxTest.ts
+++ b/tests/unit/onyxTest.ts
@@ -140,6 +140,51 @@ describe('Onyx', () => {
});
});
+ it('should deliver full collection data when connect() is followed by immediate set() of a single member in the same tick', () => {
+ const mockCallback = jest.fn();
+ const collectionKey = ONYX_KEYS.COLLECTION.TEST_CONNECT_COLLECTION;
+
+ // Write collection members directly to storage, bypassing Onyx cache values.
+ // This simulates data that exists in persistent storage but hasn't been loaded into cache yet
+ // (e.g. from a previous session).
+ return StorageMock.setItem(`${collectionKey}1`, {ID: 1, value: 'one'})
+ .then(() => StorageMock.setItem(`${collectionKey}2`, {ID: 2, value: 'two'}))
+ .then(() => StorageMock.setItem(`${collectionKey}3`, {ID: 3, value: 'three'}))
+ .then(() => {
+ // Register the keys in Onyx's key cache so getAllKeys() can discover them.
+ // We intentionally do NOT add values to the cache — only keys — to simulate
+ // data that is known to exist but whose values haven't been hydrated yet.
+ cache.addKey(`${collectionKey}1`);
+ cache.addKey(`${collectionKey}2`);
+ cache.addKey(`${collectionKey}3`);
+
+ // Connect to the collection — this starts an async storage read (microtask)
+ connection = Onyx.connect({
+ key: collectionKey,
+ waitForCollectionCallback: true,
+ callback: mockCallback,
+ });
+
+ // Immediately set a single member in the same synchronous tick.
+ // This triggers synchronous keyChanged() which calls the subscriber with a partial
+ // collection (just _1 from the cache). This sets lastConnectionCallbackData for this
+ // subscriber. The async hydration from subscribeToKey should still deliver the full
+ // collection afterwards, since the data is different.
+ Onyx.set(`${collectionKey}1`, {ID: 1, value: 'updated'});
+
+ // Wait for all async operations (storage reads from subscribeToKey) to complete
+ return waitForPromisesToResolve();
+ })
+ .then(() => {
+ // The subscriber's final state should contain ALL collection members,
+ // including _2 and _3 that were only in storage (not cache) at the time of the synchronous keyChanged call.
+ const lastCall = mockCallback.mock.calls[mockCallback.mock.calls.length - 1][0];
+ expect(lastCall).toHaveProperty(`${collectionKey}1`);
+ expect(lastCall).toHaveProperty(`${collectionKey}2`);
+ expect(lastCall).toHaveProperty(`${collectionKey}3`);
+ });
+ });
+
it('should merge an object with another object', () => {
let testKeyValue: unknown;
Could you validate if it makes sense?
|
Thanks @Krishna2323 feel free to take time off too |
This reverts commit 5d161c8.
VickyStash
changed the title
|
The reported issue turned out to be a blocker for this PR. |
|
Update:
|
|
Where is that discussion happening? |
We were discussing it with @fabioh8010 in DM. I'm going to do the perf measurements over the latest updates, and if it's all okay, then I think we can move forward |
6 participants
Details
Check the issue description for details.
Related Issues
$ Expensify/App#82871
Automated Tests
Should be covered by existing tests
Manual Tests
The E/App should work the same way as before. Let's verify basic test steps:
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios.mov
iOS: mWeb Safari
ios_web.mov
MacOS: Chrome / Safari
web.mp4