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

p2p/protocols, p2p/testing; conditional propagation of context #1648

Merged
merged 2 commits into from
Aug 16, 2019

Conversation

zelig
Copy link
Member

@zelig zelig commented Aug 3, 2019

This PR refactors the code related to context propagation within p2p/protocols.
The propagation of context is made conditional on a new protocols.Spec field DisableContext in addition to tracing.Enabled. This allows for disabling context propagation for specific protocols only.

Fixes #1575

@zelig zelig self-assigned this Aug 3, 2019
@zelig zelig added this to Backlog in Swarm Core - Sprint planning via automation Aug 3, 2019
@zelig zelig requested review from skylenet and acud August 3, 2019 11:16
Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

@zelig there's no test coverage to make sure that this exchange goes smoothly between nodes which are differently configured. are you sure this is the way you'd like to proceed with this?

@zelig zelig force-pushed the p2p-testing branch 2 times, most recently from 786796d to 3bbdca3 Compare August 14, 2019 12:43
@zelig
Copy link
Member Author

zelig commented Aug 14, 2019

@zelig there's no test coverage to make sure that this exchange goes smoothly between nodes which are differently configured. are you sure this is the way you'd like to proceed with this?

thanks.
I actually assumes all nodes have tracing if we have tracing, so no mixed nodes if the protocol spec allows it

@@ -157,6 +146,10 @@ type Spec struct {
initOnce sync.Once
codes map[reflect.Type]uint64
types map[uint64]reflect.Type

// if the protocol allows for extending the p2p msg to propagate context
// even if set to true context will propagate only if the remote peer supports it
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment looks to be right if the property would be called EnableContext . I would suggest to adjust the comment or rename DisableContext to EnableContext

@zelig zelig changed the base branch from p2p-testing to master August 14, 2019 13:56
@zelig zelig requested a review from janos August 14, 2019 14:06
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

This implementation is very nice. I do not see that previous one had explicit tests, maybe we could add some later.

p2p/protocols/protocol_test.go Outdated Show resolved Hide resolved
p2p/protocols/protocol.go Outdated Show resolved Hide resolved
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

Nice, LGTM.

@zelig zelig requested a review from acud August 14, 2019 19:03
@zelig zelig merged commit 3a13b0d into master Aug 16, 2019
Swarm Core - Sprint planning automation moved this from Backlog to Done Aug 16, 2019
@skylenet skylenet added this to the 0.5.0 milestone Aug 16, 2019
@acud acud deleted the p2p-propagate-context branch August 25, 2019 13:04
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.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

message wrapping in protocols package is not devp2p compatible
4 participants