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

Aboard Read()/Write() by SetDeadline() with past time #51

Closed
wants to merge 7 commits into from

Conversation

yudai
Copy link

@yudai yudai commented Oct 27, 2017

Hello, thanks for the handy library.

Problem Observed

When an HTTP server is served on a yamux.Session, Hijack() onhttp.ResponseWriter blocks forever, which means protocols that depend on hijacking (protocol upgrade) such as Websocket does not work with yamux.

Problem Detail

In net/http, Set(Read/Write)Deadline() with net.aLongTimeAgo is used to abort pending Read()/Write() by setting the unix epoch + 1 second. Hijack() uses this method (and I think there are some more cases) to abort Read() as well.

In the document of net.Conn, SetDeadline() is described as below:

// A deadline is an absolute time after which I/O operations
// fail with a timeout (see type Error) instead of
// blocking. The deadline applies to all future and pending
// I/O, not just the immediately following call to Read or
// Write. After a deadline has been exceeded, the connection
// can be refreshed by setting a deadline in the future.

Setting a past time is supposed to cancel any pending Read()/Write() on the connection.

However, the current implementation of yamux.Stream doesn't support this type of cancelation. Libraries expect this behavior therefore can block forever unexpectedly with yamux.

Suggested Resolution

When SetDeadline() is called, retry Read() or Write() by using the notification channels.

I think there would be no side effects caused by this change, since when you would not provide a past time to SetDeadline() when you actually don't expect cancelation by that.

@itimofeev
Copy link

Spent several days because of described problem. When do hijack() conn randomly blocked.
Thank you for PR, hope it will be merged soon.

@itimofeev
Copy link

Figured out some issue with this PR. We also need to return net.Error implementation of ErrTimeout. Or in some cases httputil.NewSingleHostReverseProxy() will return context closed error. This commit fixes problem.

stuartcarnie and others added 3 commits December 19, 2017 10:49
* flow control was assuming a `Read` consumed the entire buffer
* flow control fix reduces memory utilization when receiving large
  streams of data
* use timer pool to reduce allocations
* use static handler function pointer to avoid closure allocations
  for every frame
@hashicorp-cla
Copy link

hashicorp-cla commented Dec 6, 2019

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


1 out of 3 committers have signed the CLA.

  • preetapan
  • yudai
  • stuartcarnie

Have you signed the CLA already but the status is still pending? Recheck it.

@dadgar
Copy link
Contributor

dadgar commented Jun 9, 2020

Closed by #82

@dadgar dadgar closed this Jun 9, 2020
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.

None yet

6 participants