Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

storage: fix pyramid chunker and hasherstore possible deadlocks #1679

Merged
merged 2 commits into from
Aug 21, 2019

Conversation

acud
Copy link
Member

@acud acud commented Aug 19, 2019

This PR fixes possible deadlocks created in pyramid chunker and hasherstore.

@acud acud added the bug label Aug 19, 2019
@acud acud requested review from janos, zelig, jmozah and nolash August 19, 2019 13:14
@acud acud assigned janos and acud Aug 19, 2019
h.waitC <- ctx.Err()
select {
case h.waitC <- ctx.Err():
case <-h.quitC:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is quitC ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check L142 above in func (h *hasherStore) Wait(ctx context.Context) error

Copy link
Contributor

@nolash nolash Aug 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not so clear, but I guess the comment by the member in the owning object suggests that this is a channel for orderly shutdown.

The description of the PR suggests a fix on a deadlock. But the code seems rather to protect against closing the quitC when it's already closed, which is a panic not a deadlock? This is why I was curious

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very well observed @nolash. Yes, this PR addresses a deadlock and a panic on closing a closed channel, problems that were found while running smoke tests on new syncer, but the fixes may be applied to the master.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to second @janos, indeed the problem of close of closed channel is resolved by the Once. the change to this specific select structure is made to eliminate trivial writes to channel that are potential deadlocks

@zelig zelig merged commit a0aefcf into master Aug 21, 2019
@skylenet skylenet added this to the 0.5.0 milestone Aug 23, 2019
@acud acud deleted the hasherstore-chunker-fixes branch August 30, 2019 13:54
chadsr added a commit to chadsr/swarm that referenced this pull request Sep 23, 2019
* 'master' of github.com:ethersphere/swarm:
  chunk, storage: chunk.Store multiple chunk Put (ethersphere#1681)
  storage: fix pyramid chunker and hasherstore possible deadlocks (ethersphere#1679)
  pss: Use distance to determine single guaranteed recipient (ethersphere#1672)
  storage: fix possible hasherstore deadlock on waitC channel (ethersphere#1674)
  network: Add adaptive capabilities message (ethersphere#1619)
  p2p/protocols, p2p/testing; conditional propagation of context (ethersphere#1648)
  api/http: remove unnecessary conversion (ethersphere#1673)
  storage: fix LazyChunkReader.join potential deadlock (ethersphere#1670)
  HTTP API support for pinning contents (ethersphere#1658)
  pot: Add Distance methods with tests (ethersphere#1621)
  README.md: Update Vendored Dependencies section (ethersphere#1667)
  network, p2p, vendor: move vendored p2p/testing under swarm (ethersphere#1647)
  build, vendor: use go modules for vendoring (ethersphere#1532)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants