Page MenuHomePhabricator

[web-db] improve persisting lifecycle
ClosedPublic

Authored by kamil on Apr 3 2023, 5:46 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 5, 9:15 PM
Unknown Object (File)
Tue, Mar 5, 9:15 PM
Unknown Object (File)
Tue, Mar 5, 9:15 PM
Unknown Object (File)
Tue, Mar 5, 9:15 PM
Unknown Object (File)
Tue, Mar 5, 9:15 PM
Unknown Object (File)
Tue, Mar 5, 9:15 PM
Unknown Object (File)
Tue, Mar 5, 6:21 PM
Unknown Object (File)
Feb 13 2024, 1:00 AM
Subscribers

Details

Summary

Previous version with _throttle might cause problems when persisting will be longer than 300ms (thanks @tomek for identifying).

Change previous approach to the following algorithm:

  1. If there was a write operation and there is no ongoing persisting process (persistInProgress: false) - start persisting
  2. If there was a write operation and there is an ongoing persisting process - mark that it will need to be restarted (set persistNeeded: true).
  3. If persisting process finishes and there was a write operation (persistNeeded: true) - start it again and mark that it no longer needs to be restarted (persistNeeded: false)
  4. If persisting process finishes and there was no write operation - finish and mark that there is no ongoing persisting process (persistInProgress: false)
  5. Each time persisting process is starting set persistInProgress: true

Depends on D7288

Test Plan
  1. Test if persistence works
  2. Add sleep and check if it works for long persistence

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Apr 3 2023, 5:59 AM
tomek requested changes to this revision.Apr 4 2023, 2:57 AM
tomek added inline comments.
web/database/worker/db-worker.js
153–155 ↗(On Diff #24574)

We have a possible stack overflow here, e.g. when during each persist we set persistNeeded to true.

This revision now requires changes to proceed.Apr 4 2023, 2:57 AM

use loop instead of recursion

tomek requested changes to this revision.Apr 28 2023, 3:49 AM

I've left some comments with an alternative approach. Do you think it is more readable?

web/database/worker/db-worker.js
147 ↗(On Diff #25831)

Why do we need to use while(true) and break? Can we change the code so that while condition is used instead, e.g. while(persistNeeded)? Then we should set this flag to false as the first operation in this loop.

159 ↗(On Diff #25831)

Entering this if branch means that persistNeeded = false - why are we setting this again?

241–247 ↗(On Diff #25831)

After we change the loop to while(persistNeeded) we should modify this code

This revision now requires changes to proceed.Apr 28 2023, 3:49 AM
In D7289#227557, @tomek wrote:

I've left some comments with an alternative approach. Do you think it is more readable?

Thanks, I think your suggestions are valid

This revision is now accepted and ready to land.May 8 2023, 3:02 AM
This revision was automatically updated to reflect the committed changes.