Page MenuHomePhabricator

[web] avoid race condition while deleting database
ClosedPublic

Authored by kamil on Dec 12 2023, 1:28 AM.
Tags
None
Referenced Files
F3374084: D10306.id35119.diff
Tue, Nov 26, 1:42 PM
Unknown Object (File)
Fri, Nov 22, 9:35 PM
Unknown Object (File)
Fri, Nov 22, 8:47 PM
Unknown Object (File)
Tue, Nov 12, 10:32 AM
Unknown Object (File)
Thu, Nov 7, 8:43 PM
Unknown Object (File)
Oct 16 2024, 9:53 PM
Unknown Object (File)
Oct 16 2024, 9:53 PM
Unknown Object (File)
Oct 16 2024, 9:53 PM
Subscribers

Details

Summary

In previous code it was possible to schedule ops on not initialized DB.

Test Plan
  1. Add sleep after this line.
  2. Trigger database deletion and right after schedule some ops.
  3. Make sure there is no Database not initialized error.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Dec 13 2023, 2:03 AM
This revision is now accepted and ready to land.Dec 13 2023, 5:01 AM
michal requested changes to this revision.Dec 14 2023, 7:49 AM
michal added inline comments.
web/database/database-module-provider.js
55–64 ↗(On Diff #34516)

Wasn't the idea to put the CLEAR_SENSITIVE_DATA inside initPromise?

This revision now requires changes to proceed.Dec 14 2023, 7:49 AM

address review

web/database/database-module-provider.js
55–64 ↗(On Diff #34516)

yeah, I must've messed something while committing changes, thanks

michal requested changes to this revision.Dec 18 2023, 3:14 AM
michal added inline comments.
web/database/database-module-provider.js
62 ↗(On Diff #34713)

Does this line still make sense? schedule calls will throw an error instead of awaiting on the promise, if they happen between this line and the end of the promise.

65–70 ↗(On Diff #34713)

This should probably be moved outside of the promise. Otherwise we are leaving the db in a init state, but we will never change it to something else. If we move it outside of the promise we will probably need to also check clearDatabase in the === initSuccess case.

On the other hand maybe this can be removed? Not sure if we expect that call to init wouldn't re-init db if it's already initialized.

This revision now requires changes to proceed.Dec 18 2023, 3:14 AM
web/database/database-module-provider.js
62 ↗(On Diff #34713)

I think it can be removed, it was necessary in the previous version (before D9109) where we had separate method for clearing database

65–70 ↗(On Diff #34713)

This should probably be moved outside of the promise. Otherwise we are leaving the db in a init state, but we will never change it to something else. If we move it outside of the promise we will probably need to also check clearDatabase in the === initSuccess case.

yeah that makes sense, and good call with that

On the other hand maybe this can be removed? Not sure if we expect that call to init wouldn't re-init db if it's already initialized.

I think it's better to keep it

This revision is now accepted and ready to land.Jan 2 2024, 4:32 AM