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

net: Pipe does not properly implement the Conn interface #18170

Closed
dsnet opened this issue Dec 2, 2016 · 16 comments
Closed

net: Pipe does not properly implement the Conn interface #18170

dsnet opened this issue Dec 2, 2016 · 16 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Dec 2, 2016

The in-memory net.Conn implementations returned by net.Pipe are just thin wrappers over io.Pipe and do not properly implement net.Conn due to lack of SetDeadline support. Should net.Pipe implement deadline support?

\cc @bradfitz

@dsnet dsnet added this to the Unplanned milestone Dec 2, 2016
@dsnet dsnet changed the title net: Pipe does not satisfies Conn interface net: Pipe does not properly implement the Conn interface Dec 2, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Dec 2, 2016

I suppose they could. For testing, I assume?

@gopherbot
Copy link

CL https://golang.org/cl/34637 mentions this issue.

gopherbot pushed a commit that referenced this issue Dec 21, 2016
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>
@flyingmutant
Copy link
Contributor

Is there interest in a CL implementing deadline support (to make net.Pipe more suitable for testing, yes)?

@bradfitz
Copy link
Contributor

bradfitz commented Feb 9, 2017

@flyingmutant, yes, seems like @dsnet and I think it could. I haven't heard objections from others.

@rsc, @ianlancetaylor?

@dsnet
Copy link
Member Author

dsnet commented Feb 9, 2017

I've adjusted enough net.Conn implementations to respect deadlines, that doing the same here would be fairly straightforward. I didn't want to add complexity to net.Pipe if it isn't really being used.

@flyingmutant
Copy link
Contributor

Does flyingmutant@f63b38c (untested) look like a reasonable direction and complexity/performance tradeoff?

@bradfitz
Copy link
Contributor

bradfitz commented Feb 9, 2017

@flyingmutant, sorry, we can only look at and review code on Gerrit. See https://golang.org/doc/contribute.html

@bradfitz
Copy link
Contributor

bradfitz commented Feb 9, 2017

Also, be sure your implementation passes https://godoc.org/golang.org/x/net/nettest

@dsnet
Copy link
Member Author

dsnet commented Feb 9, 2017

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.

@flyingmutant
Copy link
Contributor

@dsnet good point about possible race, thanks.
Am I right that SetDeadline calls should only be made between Read/Write calls? If so, I don't quite understand what do you mean by "not extendable, refreshable".

@bradfitz
Copy link
Contributor

bradfitz commented Feb 9, 2017

@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.

@flyingmutant
Copy link
Contributor

@bradfitz thanks! It seems that it should be possible to SetDeadline concurrently with Read/Write calls (and pending bit in the new docs), and my naive implementation certainly does not allow that.

@flyingmutant
Copy link
Contributor

I've got a working implementation which passes nettest (with -race). However, it required modifications to io.Pipe: support for interrupting I/O much like Close does, but only for 1 operation. Is this an acceptable API addition, and is there interest in this style of implementation?

@gopherbot
Copy link

CL https://golang.org/cl/37402 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/37404 mentions this issue.

@nightlyone
Copy link
Contributor

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.

@bradfitz bradfitz modified the milestones: Go1.10, Unplanned May 15, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label May 15, 2017
gopherbot pushed a commit that referenced this issue May 24, 2017
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>
@golang golang locked and limited conversation to collaborators Oct 11, 2018
@rsc rsc unassigned dsnet Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants