Allow half-closed reads and test against TCP/TLS connections #131
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
schmichael
changed the title
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:/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.
Expanded tests to use TCP and TLS. Sorry for the huge diff, but I think this makes yamux's test suite much more realistic. There are quite a few new tools: - testConnPipe is the original io.Pipe based testing tool. Some tests still require it due to the ability to easily pause the data flow. - testConnTCP and testConnTLS create TCP and TLS connections for yamux to use. This introduces more realistic timing issues, buffering, and subtle differences in error messages. - testConnTypes is a helper to run subtests against *all* the above connection types as well as ensuring reversing the client/server sockets doesn't impact yamux (it didn't!). I didn't convert every test to it since it adds some time and noise to test runs. I also tried to formalize (client, server) as a pattern. There was a mix of orderings. Those roles are rarely meaningful to yamux, but meaningful names makes debugging easier than numbering variables.
Since switching this to test against TLS made it fail until d96c90e was applied it should be tested against all conn types. Curiously io.Pipe is unaffected by the change in half close behavior.
rboyer
reviewed
Sep 5, 2024
session_test.go
Outdated
| } | ||
| t.Cleanup(func() { _ = l.Close() }) | ||
|
|
||
| var srv net.Conn |
There was a problem hiding this comment.
This feels slightly racy. Probably better to send this net.Conn back out of the goroutine using another channel?
There was a problem hiding this comment.
Same as above, errCh synchronizes access to srv... although switching naming schemes is a bit annoying. I'll fix that up.
rboyer
reviewed
Sep 5, 2024
tgross
approved these changes
Sep 6, 2024
Comment on lines
+171
to
+175
| // See testConnType | ||
| type connTypeTest struct { | ||
| Name string | ||
| Conns connTypeFunc | ||
| } |
There was a problem hiding this comment.
This could probably be an anonymous struct in testConnTypes, which would leave one fewer top-level object for developers to figure out when reading an inherently complex test file.
rboyer
approved these changes
Sep 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.