call Stream.Reset instead of Stream.Close

[Stebalien]

May fix ipfs/kubo#6237

Basically,

1. We hang while closing a stream (because Close waits). Also, Closing a stream should cancel pending writes. go-mplex#9
2. This blocks the connection manager because it assumes that close doesn't wait.

This may also fix a stream leak.

[vyzo]

tests fail...

[Stebalien]

Yeah, we expect a nice EOF.

[vyzo]

we need the Shutdown call already.

[Stebalien]

1. We need Close to mean "close both ways" (with a sane default timeout), Reset to mean "throw everything away", and then yeah, CloseWrite and CloseRead.

[vyzo]

The test failure seems insurmountable, we'll need to change the stream API for this to work :(

[Stebalien]

Not necessarily. The test failure is due to the assumption that conn.Close() flushes. However, conn.Close() actually resets in our TCP transport as well (for efficiency) so this isn't really an issue.

[Stebalien]

But I agree this sucks.

[hsanjuan]

Maybe this is relevant here. This is how I ended up doing it in gostream :

https://github.com/hsanjuan/go-libp2p-gostream/blob/master/conn.go#L38

// Close closes the connection.
// Any blocked Read or Write operations will be unblocked and return errors.
func (c *conn) Close() error {
	if err := c.s.Close(); err != nil {
		c.s.Reset()
		return err
	}
	go pnet.AwaitEOF(c.s)
	return nil
}

If I remember well, bluntly resetting libp2p streams on Close() caused errors on the other side on situations where the stream closing was supposed to be a clean operation.

[Stebalien]

Yeah, but we need to be careful:

1. Concurrent reads aren't safe. We need to interrupt any concurrent reads (somehow) before we can do this. We should be able to do this by setting a read deadline but... we'll have to make sure to fix any/all of our multiplexers/transports (deadlines currently don't interrupt).
2. We probably want to set some timeouts/deadlines.
3. We should probably call Close() in the background as well. That will mimic a normal close(tcpFileDescriptor) call (which will let the kernel flush in the background).

IMO, the real issue here is that TCPConn.Close is actually a very fancy best-effort function that does a bunch of dirty work in the background. Ideally, even if we make Close close both directions, we wouldn't be that sloppy.

[Stebalien]

@vyzo, this is getting really critical and both the TCP and the QUIC transports reset the underlying connection on close without flushing anything (I specifically modified the TCP transport to do this to avoid unnecessary work). This bug likely just crashed our gateways and has been causing problems for a ton of people.

The only other solution I can think of is to:

1. Close the stream. We'll have to fix any bugs we have where closing streams doesn't necessarily interrupt any in-progress writes (an issue for both yamux and mplex).
2. Set a read deadline on the stream. Make deadlines interrupt and coalesce writes whyrusleeping/yamux#28 fixes read deadlines for yamux but they're still broken in mplex.
3. Wait for the current reader to exit (read lock).
4. Read on the stream waiting for an EOF or a timeout.
5. On timeout, reset.
6. Do all this in a goroutine. so we don't stall anything.

But that requires fixing multiple annoying bugs. I've started on this path (with the yamux fix) but it feels like a waste given that we don't actually make any guarantees about Close flushing.

[vyzo]

Sigh... If it has been identified as the critical bug, then we got to merge it.

[vyzo]

let's fix the test and merge it as it is a critical bug.

[Stebalien]

Sigh... If it has been identified as the critical bug, then we got to merge it.

Well, Close is stalling. I'll try shipping a patch to some of our partners to make sure this is the bug.

[vyzo]

This needs a rebase for the go mod debacle (go.sum conflicts)

[vyzo]

rebased and resolved the conflict; should be ready for merge.

[vyzo]

we also need the companion in #77, so that the connection manager doesn't kill the underlying hop relay connections when they have stop streams.

[raulk]

Initial feedback; still reviewing.

[raulk]

Previous status should've been "request changes".

fixed bug identfied, the wrapping is not an issue.