-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Description
The proposal is for conn.Context to return a context that is cancelled when the connection is closed. User's can use the context to cleanup resources with context.Afterfunc or as a context for some other work.
Motivation
It's important for users to clean up resources associated with a connection when that connection is closed. This is important for DoS resiliency and for performance. Right now the only way to do so is by registering a network.Notifiee, but that is relatively unergonomic.
There are network events, but those currently only tell you if you are (dis)connected to a peer, not if the connection you are referencing is closed.
Consider this common pattern seen in go-libp2p-kad-dht and boxo:
- Create a stream. This stream is implicitly tied to a connection.
- Store a reference to the stream to reuse the stream for multiple messages.
- subscribe to
event.EvtPeerConnectednessChangedevents to remove the
stream reference when a peer disconnects.
This pattern is subtly incorrect. Imagine you had two connections to a peer, opened a stream on connection A, and then had the remote close that connection. You are still connected, but you haven't cleaned up your stream reference. This will prevent the GC from freeing the memory associated with that connection.
Another use case is to signal early cancellation of some work. For example, autonatv2 could use this to cancel it's dial back work if the connection is closed. Since, if the connection is closed, there's no way to report back the results of the dial back.
Counter arguments
- Don't reuse streams and only keep well scoped references to a stream.
- I'm for this actually. It may require changes to Yamux to support a configurable initial send buffer. More context here: Bitswap should use one stream per message ipfs/boxo#765.
- It introduces another way to of cleaning up resources.
- This is true. I'd argue this is better than using
network.Notifiee. And, judging from existing usages, it's not often used.
- This is true. I'd argue this is better than using
Prior Art
- In the Go standard library,
*http.Requestand*tls.ClientHelloInfoboth have this method. - In quic-go, the
quic.Connectioninterface has this method.
Details
As an implementation detail, the returned context will implement it's own AfterFunc method that does not spawn a goroutine. Users can still spawn their own goroutine if needed, but for the common case of dropping some state quickly it would be nice to avoid that overhead.