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

Comments

Projects
None yet
5 participants
@dsnet
Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Dec 2, 2016

I suppose they could. For testing, I assume?

@gopherbot

This comment has been minimized.

Copy link

commented Dec 21, 2016

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

gopherbot pushed a commit that referenced this issue Dec 21, 2016

io: fix PipeWriter.Close to wake up Writes
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

This comment has been minimized.

Copy link

commented Feb 9, 2017

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

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

@rsc, @ianlancetaylor?

@dsnet

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link

commented Feb 9, 2017

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

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

@dsnet

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link

commented Feb 9, 2017

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

commented Feb 9, 2017

@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

This comment has been minimized.

Copy link

commented Feb 20, 2017

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

This comment has been minimized.

Copy link

commented Feb 24, 2017

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

@gopherbot

This comment has been minimized.

Copy link

commented Feb 24, 2017

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

@nightlyone

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2017

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 label May 15, 2017

gopherbot pushed a commit that referenced this issue May 24, 2017

vendor: add golang.org/x/net/nettest
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>

@gopherbot gopherbot closed this in e2dd8ca Oct 11, 2017

@golang golang locked and limited conversation to collaborators Oct 11, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.