multiplex: add (*Multiplex).CloseChan #87
Conversation
This commit exposes the underlying mp.closed channel to the caller, and allows the caller to receive an event on session closed.
|
Which problem does this solve? When the session is closed, the caller will notice this the next time he calls any of the functions (like opening / accepting new streams), right? |
|
In my use case, when a session is closed, another goroutine needs to remove all states associated with the session as soon as possible. Currently, I'm using a |
There was a problem hiding this comment.
Looks like we've set a precedent for exactly this kind of function with go-yamux: https://pkg.go.dev/github.com/libp2p/go-yamux#Session.CloseChan.
Just for reference, in quic-go we chose to (ab?)use a context for this: https://pkg.go.dev/github.com/lucas-clemente/quic-go#Session, thereby combining IsClosed and CloseChan into one method. Not sure if that's the nicer API choice.
| } | ||
|
|
||
| go func() { | ||
| time.Sleep(time.Second * 1) |
There was a problem hiding this comment.
Why do we need a time.Sleep here? This could be executed immediately, right?
There was a problem hiding this comment.
Yes, we don't need the .Sleep here.
There was a problem hiding this comment.
Fixed in force push
There was a problem hiding this comment.
huh, GitHub doesn't work like bitbucket does. It doesn't seem to update from force push.
There was a problem hiding this comment.
my apology, I clicked to use the wrong branch as the source. I will close this one and use the correct branch.
| case <-ctx.Done(): | ||
| t.Fatal("did not receive from CloseChan within timeout") | ||
| case <-mpb.CloseChan(): | ||
| case <-mpa.CloseChan(): |
There was a problem hiding this comment.
Should this be a second select statement, to make sure that both mpa and mpb are closed?
There was a problem hiding this comment.
You are right, I should probably do a second one to make sure that both got the "message"
There was a problem hiding this comment.
Fixed in force push
There was a problem hiding this comment.
would you mind closing this PR and instead look at #89 instead? Sorry for the mix up.
There was a problem hiding this comment.
GitHub should allow you to force push, and update the PR accordingly.
There was a problem hiding this comment.
Next time then ;)
Let’s continue in #89 for now, that’s easier.
|
Closing in favor of #89, as requested. |
This commit exposes the underlying mp.closed channel to the caller, and allows the caller
to receive an event on session closed.