Skip to content

p2p: remove Cleanup from Transport interface #3183

@ebuchman

Description

@ebuchman

In #3150 we fixed a resource leak where connections weren't being closed properly if the peer was never started (due to too many inbound peers).

The solution in that PR adds a Cleanup method to the Transport interface which must be called before calling peer.Stop. This was done to avoid having the peer magically clean things up in the transport, and to keep the Peer and Transport decoupled.

The need for this Cleanup comes from the fact that the transport implementation tracks the IP addresses of peers we're connected to, so as to not connect to a duplicate IP. So when the peer stops, it's address needs to be removed.

While decoupling of components is in general a virtue, it seems in this case the Peer and Transport are genuinely coupled. The property that the peer doesn't know about the transport sounds nice in spirit, but in practice, as far as I understand here, the implementations of Peer and Transport are already tightly wound - a particular implementation of Transport returns a particular implementation of Peer, and a particular implementation of Peer is expected to come from a particular implementation of Transport. So Peer in general doesn't need to know about Transport, but if the Transport that created it necessitates some cleanup, then it surely will.

By pushing this implicit coupling into an extra responsibility for callers to call transport.Cleanup before calling peer.Stop, we're just asking them to forget about it. If we have transports that don't require this cleanup, then we're just cluttering the Transport interface unnecessarily.

We don't need to give the peer a direct reference to the transport. It could have some cleanup closure or a channel the Transport listens for. But the current separation seems prone to misuse.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions