Skip to content

Commit

Permalink
Allow half-closed reads and test against TCP/TLS connections (#131)
Browse files Browse the repository at this point in the history
* test against tls.Conns, not pipes

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.

* locally closed streams can still read

Effectively reverts 912e296

Reasons to revert to locally closed streams being readable:
1. Matches libp2p's yamux fork: https:/libp2p/go-yamux/blob/master/stream.go#L95-L96
2. Both yamux and SPDY make it clear that a locally closed stream cannot send more data.
3. 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

* test: fix timing when using tcp

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.

* test: expand connection types tested

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.
  • Loading branch information
schmichael authored Sep 6, 2024
1 parent b5c3b44 commit 17017e9
Show file tree
Hide file tree
Showing 5 changed files with 429 additions and 180 deletions.
Loading

0 comments on commit 17017e9

Please sign in to comment.