Circuit Relay integration with basic_host #210
Conversation
|
summoning @Stebalien @whyrusleeping |
|
cc @lgierth |
p2p/host/basic/basic_host.go
Outdated
| if err != nil { | ||
| // perhaps inappropriate, but otherwise we have to change the interface | ||
| // to return an error, which will nost likely lead to a fatality anyway | ||
| panic(err) |
There was a problem hiding this comment.
Hrm... this construction feels very entangled. we need a host to create a relay that we're using to create a host? Maybe we should set the relay post construction?
There was a problem hiding this comment.
(also, panics are bad, if we need to lets just switch to returning an error here)
There was a problem hiding this comment.
Yeah, relay needs a pre-existing host to attach.
Maybe we can leave it outside the basic host construction if you prefer, although it would be nice to have it simply available through the host options.
Re: panic: I can change the interface to return an error.
There was a problem hiding this comment.
I think we want to keep the legacy interface unchanged however.
There was a problem hiding this comment.
I moved the relay attachment post construction in fd23cf6
There was a problem hiding this comment.
The inappropriate panic was squashed earlier in 7f1ffcb
|
@whyrusleeping updated the I did not change the legacy |
…tead The legacy interface stays unchanged with a panic though.
vyzo
force-pushed
the
feat/circuit-relay
branch
from
ddd47a6 to
b97f1c5
Compare
|
rebased on master and updated go-libp2p-circuit dep to 1.1.3 to keep up with upstream deps. |
p2p/host/basic/basic_host.go
Outdated
| var relayCtx context.Context | ||
| var relayCancel func() | ||
| if opts.Relay { | ||
| relayCtx, relayCancel = context.WithCancel(context.Background()) |
There was a problem hiding this comment.
I think this context is off -- it's not wired to anything, but is used to dial peers, open streams, and open relayed streams. It should be wired to something so it correctly gets cancelled when shutting down go-ipfs.
There was a problem hiding this comment.
It's loosely wired via the cancel funciton. When the basic host gets closed, it will call cancel.
Regardless, I agree that ideally this context should come from the outside and be tied to the lifetime of the host. It would take an interface change to pass it as a NewHost argument though, so that's why I didn't do that initially.
But we are changing the interface already for the error return. It's not widely used so we might as well change it further to take a context argument.
The legacy interface can pass in a context.Background().
There was a problem hiding this comment.
Added base context as parameter to NewHost in 6be81d3
don't compare peer IDs when hole punching
Integrates Circuit Relay as a transport option in basic_host.NewHost.
See #209