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

storage: fix pyramid chunker and hasherstore possible deadlocks#1679

Merged
zelig merged 2 commits intomasterfrom
hasherstore-chunker-fixes
Aug 21, 2019
Merged

storage: fix pyramid chunker and hasherstore possible deadlocks#1679
zelig merged 2 commits intomasterfrom
hasherstore-chunker-fixes

Conversation

@acud
Copy link
Copy Markdown
Contributor

@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, jmozah, nolash and zelig 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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor 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