improve negotiation flushing

[Stebalien]

First, this change exposes a Flush function so users can flush a handshake without writing or reading. We need this in libp2p to flush before closing for writing.

Second, this change flushes on Close. We can drop the read half of the handshake, but we need to send the write half. Otherwise, we could end up with the following situation:

1. A: Send data.
2. B: Receive data.
3. B: Close the stream. (no flush)
4. A: Wait for an EOF from the stream (ensure receipt of data).
5. B: ERROR: the stream was closed but no multistream header was sent.

This is slightly unfortunate as Close should ideally never block. But close is allowed to flush and the alternative is to spawn a goroutine.

[petar]

A few thoughts:

• Blocking on Close in the client is not an issue in my opinion. Close is a write-type operation and in general it is blocking (in various other implementations of io.Closer).

• If the client is used correctly, in that Close is always called, then every connection goes through a handshake, which seems to defeat the purpose of the "lazy" (i.e. handshake triggered only on use of the connection) logic. You might as well call handshake on start (in the client)?

• I am suspecting a race condition: (1) Call Write. (1a) Triggers read handshake in a goroutine (1b) Triggers and waits for a write handshake (1c) Assume read handshake has not completed by the time (1b) completes. (2) Call Read. (2a) Read handshake is not triggered (because it is already started) (2b) Proceed to reading from the connection --> this races with the ongoing read handshake.

[Stebalien]

If the client is used correctly, in that Close is always called, then every connection goes through a handshake, which seems to defeat the purpose of the "lazy" (i.e. handshake triggered only on use of the connection) logic.

The goal of the "lazy" handshake is actually:

1. I can start writing before the handshake completes.
2. I can send the multistream headers and the first message in the same packet.

In general, I don't expect users to open streams until they actually need to send data, so I'm not so concerned about always handshaking. We could walk away if we've never used the stream (never including never reading), but I'm not sure if this is worth the complexity for such an edge case.

I am suspecting a race condition: ... (2b) Proceed to reading from the connection --> this races with the ongoing read handshake.

This is protected by a sync.Once (l.rhandshakeOnce).

[Stebalien]

Blocking on Close in the client is not an issue in my opinion. Close is a write-type operation and in general it is blocking (in various other implementations of io.Closer).

I generally agree. Although usually we just close and flush async. The guarantee is: if you want to make sure the other side receives everything, call CloseWrite, then call Read to wait for an EOF. Close() acts like a normal unix TCP "close".

[petar]

@Stebalien In (2b), I mean that the ongoing "read handshake" races with https://github.com/multiformats/go-multistream/blob/master/lazyClient.go#L75

[petar]

Although usually we just close and flush async. The guarantee is: if you want to make sure the other side receives everything, call CloseWrite, then call Read to wait for an EOF. Close() acts like a normal unix TCP "close".

If you want to stick to this convention (which also seems fine), is there anything preventing you from doing it simply, e.g. like:

func (l *lazyClientConn) Close() error {
  go func() {
         _ = l.Flush()
    l.werr = l.con.Close() // sync this write somehow
  }()
  return nil
}

[Stebalien]

Read will block on the ongoing read handshake here: https://github.com/multiformats/go-multistream/blob/ff9dcd5c891b3ba194062358d52623047d85192f/lazyClient.go#L64

We always run the read handshake under this once: https://github.com/multiformats/go-multistream/blob/ff9dcd5c891b3ba194062358d52623047d85192f/lazyClient.go#L131

[Stebalien]

https://github.com/multiformats/go-multistream/pull/52#issuecomment-719041409

• I'd prefer to block than potentially oom due to launching a bunch of goroutines.
• We'd need to somehow prevent reads/writes after calling Close from blocking and/or succeeding. With that code, I'd be able to call Close() then read data with Read().

There's no reason we absolutely can't do it I just think the current solution is the lesser of two evils.