Skip to content

Await in-flight batch in IndexedDbFileSystem.flush() - #409

Closed
DenisovAV wants to merge 1 commit into
simolus3:mainfrom
DenisovAV:fix-indexeddb-flush-in-flight
Closed

Await in-flight batch in IndexedDbFileSystem.flush()#409
DenisovAV wants to merge 1 commit into
simolus3:mainfrom
DenisovAV:fix-indexeddb-flush-in-flight

Conversation

@DenisovAV

Copy link
Copy Markdown

Fixes #408.

11be8acb made flush() return _startWorkingIfNeeded(isImplicit: false) directly. That is a genuine fence when the worker is idle — _startWorkingIfNeeded now awaits _performWrites — and it removed the marker's overhead. But while a batch is already being written, _startWorkingIfNeeded takes no branch and its future completes immediately, so flush() returns before that batch reaches IndexedDB. With writeAutomatically: true, _isWorking is set synchronously by the implicit start in _submitWork, so this is the ordinary state right after any write.

This keeps the idle fast path and restores the marker only when busy:

Future<void> flush() {
  if (!_isWorking) {
    return _startWorkingIfNeeded(isImplicit: false);
  }
  return _submitWorkFunction((_) async {}, 'flush');
}

Why the marker is sound: _FunctionWorkItem does not override insertInto, so it always lands at the tail of _pendingWork and is never merged or skipped. When the running batch finishes, whenComplete restarts work, and the next batch carries every write submitted before the flush plus the marker last. With writeAutomatically: false the running batch can only have been started explicitly, so its restart also uses isImplicit: false and proceeds. This is the same shape close() already uses.

On testing

I haven't added a test, and want to be upfront about why rather than add one that passes for the wrong reason. The obvious check — flush, then read the data back through a fresh IndexedDbFileSystem or AsynchronousIndexedDbFileSystem — doesn't discriminate: an IndexedDB transaction that overlaps a pending readwrite transaction on the same store waits for it, so the read sees the data whether or not flush() actually awaited. A test that asserts on the number of event-loop turns before flush() resolves would catch it (it reads 0 on 3.5.2 and 7 on 3.3.3 in headless Chrome), but that is fragile. If you have a preferred way to observe work-queue state from a test, I'm happy to add one.

Verified: dart analyze clean on the changed file, dart format unchanged.

11be8ac made flush() return _startWorkingIfNeeded directly, which is a genuine fence when the worker is idle but takes no branch while a batch is already being written, so flush completes before that batch reaches IndexedDB.

Keep the idle fast path and queue a no-op marker only when busy. _FunctionWorkItem does not override insertInto, so the marker always lands at the tail of _pendingWork and resolves after the work ahead of it is written. Fixes simolus3#408.

@simolus3 simolus3 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR. I agree the current state is buggy, but we can likely clean up the fix a bit.

Instead of adding a separate branch here, can we make flush() return the future associated with a prior work item? The easiest way to do that would be to replace bool _isWorking with a Future<void>? _currentWork.

In flush(), we then await _currentWork or _startWorkingIfNeeded if it's unset. This avoids scheduling a bogus work item, which also starts an IndexedDB transaction (so it's not free).

@simolus3 simolus3 closed this in bf00f0d Sep 12, 2026
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.

IndexedDbFileSystem.flush() does not await a batch already in flight (regression in 3.4.0)

2 participants