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

Add support for closing write ends of streams #84

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eandre
Copy link

@eandre eandre commented Sep 14, 2020

When sending payloads of unknown length over a Stream
and expecting the server to read it to completion before
emitting a response (such as forwarding a byte stream),
one difficulty with yamux is communicating that the byte
stream's end has been reached.

One approach is to introduce a higher-level protocol,
but when the byte stream is of unknown size this requires
essentially reimplementing large parts of yamux's framing
protocol.

A simpler solution is to communicate that EOF has been reached.
Yamux provides this capability through (*Stream).Close,
but it closes both the read and the write ends, which then
prevents the client from reading the response from the server.

This change introduces a new method, (*Stream).CloseWrite,
which only closes the write end of the stream. When encountered
on the other end, it sets a flag that the remote's write end has been
closed and begins returning EOF from any reads after the receive
buffer has been exhausted.

When sending payloads of unknown length over a Stream
and expecting the server to read it to completion before
emitting a response (such as forwarding a byte stream),
one difficulty with yamux is communicating that the byte
stream's end has been reached.

One approach is to introduce a higher-level protocol,
but when the byte stream is of unknown size this requires
essentially reimplementing large parts of yamux's framing
protocol.

A simpler solution is to communicate that EOF has been reached.
Yamux provides this capability through (*Stream).Close,
but it closes both the read and the write ends, which then
prevents the client from reading the response from the server.

This change introduces a new method, (*Stream).CloseWrite,
which only closes the write end of the stream. When encountered
on the other end, it sets a flag that the remote's write end has been
closed and begins returning EOF from any reads after the receive
buffer has been exhausted.
@hashicorp-cla
Copy link

hashicorp-cla commented Sep 14, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@evanphx evanphx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment about the switch, but also can you do a little testing about what happens when a newer yamux sends CloseWrite to an older one that doesn't support the flag? We haven't added new flags to yamux in so many years, it's something we need to consider here.

case streamEstablished:
goto SEND_CLOSE

case streamLocalClose:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean that seeing streamLocalClose means we do nothing and continue after the switch but Go makes it hard to here if that's the case or if it's a programming error. Could you add a comment to the bottom saying something like "ok, handle normally" or something? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants