multiformats / go-multistream

an implementation of the multistream protocol in go
MIT License
40 stars 27 forks source link

write asynchronously when handshaking #29

Closed Stebalien closed 6 years ago

Stebalien commented 6 years ago

Given that both sides now try to write the multistream handshake out before reading, we need to do it asynchronously. Otherwise, we can deadlock (depending on the protocol).

In practice, this shouldn't be an issue but I'm not sure if we can really guarantee that.

This fix does cost us an ephemeral goroutine but I'm not sure if we can get away from that.

Stebalien commented 6 years ago

Note: I hit this when testing with a pipe (in a different package).

Stebalien commented 6 years ago

/cc @marten-seemann.

whyrusleeping commented 6 years ago

I initially wrote things this way to eplicitly avoid having an extra goroutine like this. But i guess 'goroutines are cheap', and we're opening streams way less often than we used to (though you want to change things mr @Stebalien )

marten-seemann commented 6 years ago

Why could we deadlock when both peers are writing to the stream at the same time? Which stream muxers does this apply to?

Stebalien commented 6 years ago

@whyrusleeping we'll be using a different protocol (where the client will always speak first). Here:

  1. In the past, the server spoke first. That doesn't work with protocols like QUIC that don't "open" the stream until data is sent.
  2. Now, they both speak at the same time. We can't change the server's behavior as that'd break old clients.

@marten-seemann

Why could we deadlock when both peers are writing to the stream at the same time? Which stream muxers does this apply to?

The problem is that we aren't reading. We could also just assume that there's a small write buffer.

Stebalien commented 6 years ago

Note: the lazy negotiation streams (the fast path ones) already launch goroutines so I'm not so concerned here.

marten-seemann commented 6 years ago

I think it’s a valid assumption that we have initially have enough flow control credit to write a protocol ID, so I’d prefer to keep things simple here.

Stebalien commented 6 years ago

I think it’s a valid assumption that we have initially have enough flow control credit to write a protocol ID...

This is more about buffering than control flow credit. For example, in a theoretical (but planned) "local" transport, each "stream" would just be a net.Pipe. We could have a buffering requirement but I'd rather not, especially because net.Pipe doesn't and go had rejected proposals to add a buffer multiple times.

Stebalien commented 6 years ago

I'm going with this for now. It's the conservative approach and we can loosen it up if we find performance issues.