PhysarumSM / service-manager

LCA and Proxy
Apache License 2.0
0 stars 0 forks source link

Look into how to properly terminate connections in LCAAllocatorHandler #24

Closed t-lin closed 4 years ago

t-lin commented 4 years ago

An open question is whether to call stream.Reset() or leave it as stream.Close() (the current implementation). The documentation (https://godoc.org/github.com/libp2p/go-libp2p-core/mux#MuxedStream) seems to indicate that Close() is a half-duplex close (i.e. one-sided).

The answer might depend on whether stream objects between two peers are re-used for each request or not. If they are re-used, would calling Reset() prevent the same client from re-connecting in the future, and force it to open a new connection and incurring latency? Need to do further testing and investigating.

t-lin commented 4 years ago

Stream objects are not re-used for each request, and calling Reset() only closes the stream, not the connection.

There's definitely a stream leak issue in the current implementation of proxy / LCA manager so long as the underlying connection to a peer remains active. This can be seen by printing out the number of streams within a connection before and after calling NewStream() within AllocService() and repeatedly triggering it.

The godoc indicates that Close()'ing a stream on one end is a half-duplex close, so one would assume the other end would automatically be aware of the close. However, digging into the source code (https://github.com/libp2p/go-libp2p-swarm/blob/master/swarm_stream.go), it seems like an end would only become aware of the other end closing if it calls Read() and detects an EOF. Ideally each side should call Read()s until an EOF, prior to calling Close() for itself. It's unclear how to explicitly signal to the other side of one's intention to close (i.e. trigger the EOF).

To be safe (and likely easier to do) just simply call Reset() once we're past the point in the code where we need the stream at all.