hyperdrive-next icon indicating copy to clipboard operation
hyperdrive-next copied to clipboard

Add test for now-fixed race condition

Open HDegroote opened this issue 2 years ago • 4 comments

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 access this.blobs.core, which is swallowed in its try-catch (and this.db.feed and this.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.

HDegroote avatar Jan 25 '23 17:01 HDegroote

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)

HDegroote avatar Jan 25 '23 17:01 HDegroote

still relevant @HDegroote ?

mafintosh avatar May 04 '23 09:05 mafintosh

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

HDegroote avatar May 04 '23 10:05 HDegroote

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

HDegroote avatar May 22 '23 17:05 HDegroote