search

hashicorp / yamux

Golang connection multiplexing library
Mozilla Public License 2.0
2.25k stars 236 forks source link

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

Closed yudai closed 4 years ago

yudai commented 7 years ago

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 commented 7 years ago

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

itimofeev commented 7 years ago

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.

hashicorp-cla commented 5 years ago

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.

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

dadgar commented 4 years ago

Closed by https://github.com/hashicorp/yamux/pull/82