Skip to content

Stop a failed box open leaking its error into the zone - #320

Open
LahaLuhem wants to merge 3 commits into
IO-Design-Team:mainfrom
LahaLuhem:bugfix/#319-error-leak
Open

Stop a failed box open leaking its error into the zone#320
LahaLuhem wants to merge 3 commits into
IO-Design-Team:mainfrom
LahaLuhem:bugfix/#319-error-leak

Conversation

@LahaLuhem

Copy link
Copy Markdown

Fixes #319.

When an openBox fails and you retry it right away, the retry's error arrives twice. Once at whoever awaited it, which is right, and once as an unhandled error in the zone, which isn't.

What was going on

A failed open kicks off newBox?.close().ignore() and doesn't wait for it. close() awaits keystore.close() first, so unregisterBox lands a microtask later, by which point the retry has already put its own future in _openingBoxes. The dead box then removes it, and nobody is left to handle that future's error.

Traced with prints on the failing run:

[hive] finally found own future: true      <- attempt 1, fine
caller caught #1
[hive] unregisterBox(things) dropped opening future: true   <- eats attempt 2's future
[hive] finally found own future: false     <- attempt 2 has nothing to ignore
caller caught #2
escaped to zone = 1

That lines up with everything the issue reported: the first attempt is always clean, N attempts leak N-1, and a single microtask between the calls makes it disappear.

The change

Commit What
eca28ff the test, red on its own
a701708 stop unregisterBox clearing _openingBoxes
10a08cc only unregister the box that is actually registered under that name

The second one is a deleted line. _openBox already removes its own entry in a finally, on success and on failure alike, so anything still sitting in _openingBoxes belongs to an open that is running right now. Clearing it there was never doing a useful job.

The third is a sibling of the same problem. A box closing slowly could deregister a live box that took its name in the meantime. I could not get that one to misbehave in a test, since keystore.close() is one microtask while a box open does real file work, so the ordering happens to land the right way. It is guarded rather than left to luck.

Heads up: the third commit's guard also happens to cover #319 on its own, because it returns early in exactly the failed-open case. So the two are not independent fixes, they overlap.

Tests

Three added in hive_impl_test.dart:

  • one failed open leaks nothing, which passes before the fix too and keeps the pair honest
  • a retried failed open leaks nothing, which is the regression
  • unregistering for a box that isn't the registered one leaves the live one alone
Red before, green after

At eca28ff, before the fix:

HiveImpl .openBox() open failure a failed open does not leak its error to the zone   [ok]
HiveImpl .openBox() open failure retrying a failed open does not leak its error to the zone [E]
  Expected: empty
    Actual: [FormatException:FormatException: cannot decode this record]

At a701708 and after, both pass.

The first commit is deliberately red so the bug is visible in history. CI builds the merge result rather than each commit, so the checks here should be green. Squash it into the second if you would rather not have a red commit in the log.

Checked with dart analyze --fatal-infos, dart format --set-exit-if-changed, and 590 tests on the VM. Also ran box_base_test on chrome/dart2js, since the third commit touches code shared with web.

_openBox removes its own entry in a finally, so anything left in
_openingBoxes belongs to an open that is still running. Removing it
there dropped that future's error unhandled and hid the open from
the next caller.
A box closing after a later open already took its name would
otherwise deregister the live one.
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.

Retrying a failed openBox leaks the error into the zone as well as to the caller

1 participant