Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow half-closed reads and test against TCP/TLS connections #131

Merged
merged 9 commits into from
Sep 6, 2024

Commits on Sep 5, 2024

  1. 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.
    schmichael committed Sep 5, 2024
    Configuration menu
    Copy the full SHA
    bc6e2bf View commit details
    Browse the repository at this point in the history
  2. locally closed streams can still read

    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
    schmichael committed Sep 5, 2024
    Configuration menu
    Copy the full SHA
    f14ea25 View commit details
    Browse the repository at this point in the history
  3. test: rename variables and expand comments

    0 functional changes.
    schmichael committed Sep 5, 2024
    Configuration menu
    Copy the full SHA
    c1ae90b View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    40e0693 View commit details
    Browse the repository at this point in the history
  5. 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.
    schmichael committed Sep 5, 2024
    Configuration menu
    Copy the full SHA
    189d4ef View commit details
    Browse the repository at this point in the history
  6. 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.
    schmichael committed Sep 5, 2024
    Configuration menu
    Copy the full SHA
    5967702 View commit details
    Browse the repository at this point in the history
  7. test TestHalfClose against all conn types

    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.
    schmichael committed Sep 5, 2024
    Configuration menu
    Copy the full SHA
    3e8d1f5 View commit details
    Browse the repository at this point in the history
  8. remove useless comment

    schmichael committed Sep 5, 2024
    Configuration menu
    Copy the full SHA
    93b744c View commit details
    Browse the repository at this point in the history
  9. Configuration menu
    Copy the full SHA
    f1a66c0 View commit details
    Browse the repository at this point in the history