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
net: Pipe does not properly implement the Conn interface #18170
Comments
I suppose they could. For testing, I assume? |
CL https://golang.org/cl/34637 mentions this issue. |
Since commit cc62bed (CL 994043) the pipe deadlock when doing Read+Close or Write+Close on same end was fixed, alas with test for Read+Close case only. Then commit 6d6f338 (CL 4252057) made a thinko: in the writer path p.werr is checked for != nil and then err is set but there is no break from waiting loop unlike break is there in similar condition for reader. Together with having only Read+Close case tested that made it to leave reintroduced Write+Close deadlock unnoticed. Fix it. Implicitly this also fixes net.Pipe to conform to semantic of net.Conn interface where Close is documented to unblock any blocked Read or Write operations. No test added to net/ since net.Pipe tests are "Assuming that the underlying io.Pipe implementation is solid and we're just testing the net wrapping". The test added in this patch should be enough to cover the breakage. Fixes #18401 Updates #18170 Change-Id: I9e9460b3fd7d220bbe60b726accf86f352aed8d4 Reviewed-on: https://go-review.googlesource.com/34637 Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
Is there interest in a CL implementing deadline support (to make |
@flyingmutant, yes, seems like @dsnet and I think it could. I haven't heard objections from others. |
I've adjusted enough |
Does flyingmutant@f63b38c (untested) look like a reasonable direction and complexity/performance tradeoff? |
@flyingmutant, sorry, we can only look at and review code on Gerrit. See https://golang.org/doc/contribute.html |
Also, be sure your implementation passes https://godoc.org/golang.org/x/net/nettest |
A brief glance at your implemention seems to show that deadlines are not extendable or refreshable. There also seems to allow for possible raciness when deadlines occur on a read or write call since the buffers are still accessed by the opFn. Can review if you submit a Gerrit CL. |
@dsnet good point about possible race, thanks. |
@flyingmutant, we spent some time polishing the docs on the Conn interface during Go 1.8. See if this addresses your questions: https://beta.golang.org/pkg/net/#Conn If not, we should fix the docs. |
@bradfitz thanks! It seems that it should be possible to |
I've got a working implementation which passes nettest (with |
CL https://golang.org/cl/37402 mentions this issue. |
CL https://golang.org/cl/37404 mentions this issue. |
Instead of making net.Pipe more complex, adding a decent abstraction of http://man7.org/linux/man-pages/man2/socketpair.2.html and using that might be more rewarding in terms of realistic test scenarios. That also allows way more settings enabling even more test scenarios. |
Adds golang.org/x/net/nettest at revision 9773060888fba93b172cedcd70127db1ab739bd1. This allows us to test net.Conn implementations for compliance. Updates #18170 Change-Id: I8d3d3430b0a1abc83513180a677c39ee39303f5a Reviewed-on: https://go-review.googlesource.com/37404 Reviewed-by: Mikio Hara <mikioh.mikioh@gmail.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Mikio Hara <mikioh.mikioh@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
The in-memory
net.Conn
implementations returned bynet.Pipe
are just thin wrappers overio.Pipe
and do not properly implementnet.Conn
due to lack ofSetDeadline
support. Shouldnet.Pipe
implement deadline support?\cc @bradfitz
The text was updated successfully, but these errors were encountered: