hyperdrive-next
hyperdrive-next copied to clipboard
Add test for now-fixed race condition
Something odd happens when a drive is closed before it has managed to complete _openBlobsFromHeader
(for example because the drive is not yet available locally):
- Old behaviour:
_close()
throws when trying to accessthis.blobs.core
, which is swallowed in its try-catch (andthis.db.feed
andthis.corestore
never get closed) - New behaviour: avoids TypeErrors, so everything is always closed (I put '?' everywhere, but the only necessary one is the one after
this.blobs
)
No idea if this is a race condition, it depends on the contract of close()
. If the contract is 'when close resolves, the drive's blobs, db-feed and corestore are closed', then it is indeed a race condition
Note: I also added safetyCatch instead of swallowing, because it makes it easier to detect such errors, but this is a very breaking change, so not so sure if that's a good idea.
While looking into this I also noticed that this line: https://github.com/holepunchto/hyperdrive-next/blob/973bc95cdaa630a40c523d57f8f1443756bc9940/index.js#L119 is probably not relevant (I don't see how it could be true).
However, assuming it is relevant, then I think there's another race condition possible: https://github.com/holepunchto/hyperdrive-next/blob/973bc95cdaa630a40c523d57f8f1443756bc9940/index.js#L129-L131
I believe that the ready()
should go after the assignment to this.blobs
. Else it's possible that this.blobs
is briefly assigned another value which is then overwritten when blobs.ready()
completes.
(or, alternatively, adding another if (this.blobs) return true
before the assignment)
still relevant @HDegroote ?
still relevant @HDegroote ?
This test now passes in main, so the underlying issue was already fixed in the mean time. I updated the PR to only include the additional test (on closing a Hyperdrive before its blobs were loaded), which I think is a nice edge case to test, but just closing this PR is fine too
Fixed a pending issue of accessing private corestore._closing
(thnx to just-published corestore version which uses ready resource), so adding this test should now be good to merge