Skip to content

Conversation

@whizzzkid
Copy link
Contributor

the test case passes, but it shouldn't as the lock is still acquired? Reading more into how mortice works, maybe the test is not running in the WebWorker context.

:thinking-face:

@achingbrain
Copy link
Member

I haven't run this test but from what I can see this passes because getFirstEntry breaks out of the await for..of which ends the generator and causes the lock to be released.

I think you might be able to create a deadlock by trying to acquire a write lock during a read lock:

// getMany opens a (shared) read lock
for await (const block of helia.blockstore.getMany(someAsyncCIDEmitter)) {
  // gc opens a (exclusive) write lock, but needs to wait for the read lock to be released
  await helia.gc()
}

To break the deadlock there should be some sort of timeout mechanism used while trying to acquire a lock - passing a AbortSignal.timeout for example.

@achingbrain
Copy link
Member

Again, I haven't run it but awaiting gc before getFirstEntry is called might do the same thing:

const retrieved = await storage.getMany(cidsGenerator())
await helia.gc() // might cause deadlock?
const firstEntryFromRetrieved = await getFirstEntry(retrieved)

@BigLep
Copy link
Contributor

BigLep commented May 1, 2023

@whizzzkid : what is motivating this work? I'm not seeing any discussion in the PR or to a linked issue. I'm not saying we shouldn't do the work, but it would be good to have the context on why we're prioritizing this currently.

whizzzkid added 2 commits May 18, 2023 01:39
* main:
  chore(release): 1.0.4 [skip ci]
  docs: add usage example (#97)
  chore: bump aegir from 38.1.8 to 39.0.4 (#111)
  chore: Update CODEOWNERS [skip ci]
  docs: add system diagram (#93)
  deps(dev): bump libp2p from 0.43.4 to 0.44.0 (#96)
Signed-off-by: Nishant Arora <[email protected]>
@BigLep
Copy link
Contributor

BigLep commented Aug 28, 2023

Is this still relevant to keep open given 4 months in draft?

achingbrain added a commit that referenced this pull request Jan 8, 2024
achingbrain pushed a commit that referenced this pull request Jan 8, 2024
## [@helia/ipns-v1.1.4](https://github.com/ipfs/helia-ipns/compare/@helia/ipns-v1.1.3...@helia/ipns-v1.1.4) (2023-09-11)

### Trivial Changes

* update project config ([#99](ipfs/helia-ipns#99)) ([a704fdc](ipfs/helia-ipns@a704fdc))

### Dependencies

* bump multiformats from 11.0.2 to 12.0.1 ([#57](ipfs/helia-ipns#57)) ([6f93e51](ipfs/helia-ipns@6f93e51))
* **dev:** bump aegir from 39.0.13 to 40.0.8 ([#65](ipfs/helia-ipns#65)) ([174987b](ipfs/helia-ipns@174987b))
@achingbrain
Copy link
Member

I'm going to close this because there's been no movement in almost a year. I think yes, if you mis-use async generators you can get yourself into trouble but this is a bug in the consumer's code, not the internal implementation.

@achingbrain achingbrain deleted the fix/locking branch July 31, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: 🏃‍♀️ In Progress
Status: 🔎 In Review

Development

Successfully merging this pull request may close these issues.

4 participants