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.

[Stebalien]

Replacing this with an explicit LazyConn is technically breaking, but nothing uses it (and the interface was quite useless...).

[Stebalien]

This is breaking, and it's a problem. Currently, go-libp2p-core expects this to return an io.ReadWriteCloser.

However, there is no explicit dependency relationship between this module and go-libp2p-core (intentionally).

[Stebalien]

So, I've reverted this. I'm going to do type assertions in go-libp2p where necessary.

[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:

go-multistream/lazyClient.go

Line 64 in ff9dcd5

l.rhandshakeOnce.Do(func() {

We always run the read handshake under this once:

go-multistream/lazyClient.go

Line 131 in ff9dcd5

go l.rhandshakeOnce.Do(l.doReadHandshake)

[Stebalien]

#52 (comment)

• 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.

[aschmahmann]

Left a comment, this makes sense to me. Although my familiarity with this part of the code base and it's rough edges isn't as good as I'd like it to be.

@raulk any thoughts before @Stebalien starts moving this along through the stack?

---

@Stebalien do you mind if while we propagate these changes through go-libp2p (and go-ipfs) we use stacks of PRs instead of merging + releasing at each step? It'll give us the most test coverage since we have test coverage throughout the stack,