Remove goprocess from Host #865
Conversation
p2p/host/basic/basic_host.go
Outdated
| select { | ||
| case <-hostCtx.Done(): | ||
| h.teardown() | ||
| } |
There was a problem hiding this comment.
| select { | |
| case <-hostCtx.Done(): | |
| h.teardown() | |
| } | |
| <-hostCtx.Done() | |
| h.teardown() |
There was a problem hiding this comment.
This code has been removed.
p2p/host/basic/basic_host.go
Outdated
| @@ -812,7 +823,8 @@ func (h *BasicHost) Close() error { | |||
| // This: | |||
| // 1. May be called multiple times. | |||
| // 2. May _never_ be called if the host is stopped by the context. | |||
There was a problem hiding this comment.
comment maybe out of date now?
There was a problem hiding this comment.
Have removed this comment.
| @@ -105,13 +106,15 @@ func NewIDService(ctx context.Context, h host.Host, opts ...Option) *IDService { | |||
| userAgent = cfg.userAgent | |||
| } | |||
|
|
|||
| hostCtx, cancel := context.WithCancel(context.Background()) | |||
There was a problem hiding this comment.
being able to have the ID service run within a larger context continues to seem valuable.
There was a problem hiding this comment.
I don't see why. It's a separate service and exposes a Close function we can call to shut it down. This is similar to the DHT<-> Routing Table relationship and the same idea works fine for us there.
What are your concerns here ?
There was a problem hiding this comment.
if a consumer is spinning up a bunch of separate services, it's nice to have them all wired up to a central context, and then cancel that context to shut things down, rather than explicitly calling close on each service.
There was a problem hiding this comment.
I see where you are coming from but we've had problems in the past using contexts for cancellation across service boundaries. Please take a look at libp2p/go-libp2p-kbucket#50 (comment) & the linked discussions in that comment.
The most important benefit to me is being able to control the order in which the services, sub-components etc. are shut down.
| currid: make(map[network.Conn]chan struct{}), | ||
| observedAddrs: NewObservedAddrSet(ctx), | ||
| observedAddrs: NewObservedAddrSet(hostCtx), |
There was a problem hiding this comment.
Should we also have a Close for the ObservedAddrSet that we can call in IDService.Close()
|
@Stebalien Now that I think about it, if we had many go-routines in the |
p2p/host/basic/basic_host.go
Outdated
| @@ -343,7 +335,12 @@ func makeUpdatedAddrEvent(prev, current []ma.Multiaddr) *event.EvtLocalAddresses | |||
| return &evt | |||
| } | |||
|
|
|||
| func (h *BasicHost) background(p goprocess.Process) { | |||
| func (h *BasicHost) background() { | |||
| h.refCount.Add(1) | |||
There was a problem hiding this comment.
Unfortunately, this is racy. See the note on https://golang.org/pkg/sync/#WaitGroup.Add.
We have to call Add before we go async.
There was a problem hiding this comment.
(otherwise background might not start running till after the user calls Close)
There was a problem hiding this comment.
I have no idea how I missed this. Thanks for the catch.
p2p/host/basic/basic_host.go
Outdated
| func (h *BasicHost) background() { | ||
| h.refCount.Add(1) | ||
| defer func() { | ||
| h.refCount.Done() |
p2p/protocol/identify/id.go
Outdated
| func (ids *IDService) handleEvents() { | ||
| ids.refCount.Add(1) |
p2p/protocol/identify/id.go
Outdated
| func (ids *IDService) handleEvents() { | ||
| ids.refCount.Add(1) | ||
| sub := ids.subscription | ||
| defer func() { | ||
| _ = sub.Close() | ||
| // drain the channel. | ||
| for range sub.Out() { |
There was a problem hiding this comment.
note: we no longer need this (close will do it for us). We can simplify to:
defer ids.refCount.Done()
defer sub.Close()There was a problem hiding this comment.
Had a look at Sub.Close() in go-eventbus. You are right. This is done.
| currid: make(map[network.Conn]chan struct{}), | ||
| observedAddrs: NewObservedAddrSet(ctx), | ||
| observedAddrs: NewObservedAddrSet(hostCtx), |
|
@aarshkshah1992 LGTM. Could you make sure the test failures are that known issue then merge if so. |
|
@Stebalien The test that fails is #854. |
@Stebalien
For #862 .