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.feedandthis.corestorenever 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