Skip to content

Add IDBFS.onAutoPersistStateChanged callback. Fixes flakiness in browser.test_fs_idbfs_sync_autopersist#26895

Merged
juj merged 4 commits into
emscripten-core:mainfrom
juj:give_autopersist_time
May 9, 2026
Merged

Add IDBFS.onAutoPersistStateChanged callback. Fixes flakiness in browser.test_fs_idbfs_sync_autopersist#26895
juj merged 4 commits into
emscripten-core:mainfrom
juj:give_autopersist_time

Conversation

@juj
Copy link
Copy Markdown
Collaborator

@juj juj commented May 8, 2026

Add IDBFS.onAutoPersistStateChanged callback that can be used to detect when IDBFS autopersistence operations start and finish.

Fixes test failures in test browser.test_test_fs_idbfs_sync_autopersist, by giving the persistence logic time to complete before quitting the first phase of test.

Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Strange that we don't have this test marked as @flaky?

Comment thread test/fs/test_idbfs_sync.c Outdated
@sbc100 sbc100 changed the title Fix flakiness in browser.test_test_fs_idbfs_sync_autopersist Fix flakiness in browser.test_fs_idbfs_sync_autopersist May 8, 2026
Comment thread test/fs/test_idbfs_sync.c Outdated
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm.

I think a public callback for "sync is now complete" might be a good thing to add at some point, but no need to block this PR on that.

@juj juj changed the title Fix flakiness in browser.test_fs_idbfs_sync_autopersist Add IDBFS.onAutoPersistStateChanged callback. Fixes flakiness in browser.test_fs_idbfs_sync_autopersist May 8, 2026
@juj
Copy link
Copy Markdown
Collaborator Author

juj commented May 8, 2026

On further thought, I added a callback to IDBFS to provide autopersist completion events. Please take a new review round.

@juj
Copy link
Copy Markdown
Collaborator Author

juj commented May 9, 2026

Does this look good to go?

Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Nice.

I wonder if you should maybe try to be modern and use a promise here instead of a callback?

e.g. the user would write IDBFS.autoPersistComplete().then(...)

Or

...
await IDBFS.autoPersistComplete();
doCleanUp();
...

Comment thread test/fs/test_idbfs_sync.c Outdated
Comment thread test/fs/test_idbfs_sync.c
callUserCallback(_finish);
}
}
});
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So this would just be EM_ASM(IDBFS.autoPersistComplete().then(finish));

Then no auto-persist is outstanding the promise would just resolve right away.

@juj
Copy link
Copy Markdown
Collaborator Author

juj commented May 9, 2026

I wonder if you should maybe try to be modern and use a promise here instead of a callback?

Promise only resolves once, but this callback fires every time there is an autopersist start or end. So caller would have to juggle repeat promise registrations after every time their callback fires, and there would have to be a mechanism to pregenerate those promises. I don't think that would work too well?

@sbc100
Copy link
Copy Markdown
Collaborator

sbc100 commented May 9, 2026

I wonder if you should maybe try to be modern and use a promise here instead of a callback?

Promise only resolves once, but this callback fires every time there is an autopersist start or end. So caller would have to juggle repeat promise registrations after every time their callback fires, and there would have to be a mechanism to pregenerate those promises. I don't think that would work too well?

Fair enough.

@juj juj force-pushed the give_autopersist_time branch from 2939c26 to fab28f3 Compare May 9, 2026 21:57
@juj juj enabled auto-merge (squash) May 9, 2026 21:57
@juj juj disabled auto-merge May 9, 2026 23:13
@juj juj merged commit e6ebce4 into emscripten-core:main May 9, 2026
27 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants