Allow half-closed reads and test against TCP/TLS connections#131
Merged
Conversation
Specifically to debug hashicorp/nomad#23305 but tests should probably run against multiple net.Conn implementations as yamux is sensitive to net.Conn behaviors such as concurrent Read|Write|Close and what errors are returned.
Effectively reverts 912e296 Reasons to revert to locally closed streams being readable: Matches libp2p's yamux fork: https://github.com/libp2p/go-yamux/blob/master/stream.go#L95-L96 Both yamux and SPDY make it clear that a locally closed stream cannot send more data. SPDY explicitly supports unidirectional streams where one peer is closed (readonly) from the beginning: https://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3-1/#46-unidirectional-streams
0 functional changes.
TestSession_PartialReadWindowUpdate asserts that large sends can cause window updates. When using io.Pipes the server recieves the window update nearly synchronously during the client's Read call. Using tcp sockets adds just enough latency to reliably lose the race for observing the window update on the server. Added a sleep to ensure the server has time to read and apply the window update from the client. I also added a number of comments and non-functional cleanups. The time.Sleep() is the only functional change.
Summary
Details
In an attempt to debug hashicorp/nomad#23305 I switched tests from using
io.Pipetotls.Connfor transports.While this did not uncover the root cause of 23305, it did catch a case where the implementation did not follow the yamux or SPDY specs: after calling Stream.Close, you can still call Stream.Read. Reads on half-closed streams had been explicitly disallowed in 912e296 for unclear reasons. Even at the time
TestHalfClosewas unaffected by this change. Switching the test from io.Pipe to tls.Conn uncovered the bug. See https://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3-1/#236-stream-half-close for details.Individual commits have more details and are hopefully grouped reasonably. In particular 5bda672 describes the new test helpers.
#127
Sadly no test results changed before or after #127 was applied. This is not entirely surprising as we already suspected tests did not cover the changes being made.