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

proposal: net: add BufferedPipe (buffered Pipe) #34502

Open
iangudger opened this issue Sep 24, 2019 · 32 comments
Open

proposal: net: add BufferedPipe (buffered Pipe) #34502

iangudger opened this issue Sep 24, 2019 · 32 comments

Comments

@iangudger
Copy link
Contributor

@iangudger iangudger commented Sep 24, 2019

If one wants to connect two Go libraries which use the net interfaces, the only cross platform way to do it with the standard library is to use the loopback. The only in-process way to do it is to create a socket pair with syscall.Socketpair and go from FD -> *os.File -> net.Conn. This is a fairly manual process, and while local to the machine and fully in-memory, does use a non-cross-platform kernel feature to do the heavy lifting.

A native Go transport would be more ideal, but expecting users to implement their own is maybe not reasonable as the net.Conn and net.Listener interfaces are difficult to implement correctly. I propose that we add a canonical and cross-platform in-memory transport in either net or x/net.

Further, I propose the following interface:

func NewConnPair(addr1, addr1 net.Addr) (net.Conn, net.Conn)
func NewPacketConnPair(addr1, addr1 net.Addr) (net.PacketConn, net.PacketConn)
func NewDialerListenerPair(dialAddr, listenAddr net.Addr) (func(context.Context) (net.Conn, error), net.Listener)

Maybe it would be better to create new exported types and return those instead of the interfaces (e.g. MemoryConn, MemoryPacketConn, and MemoryListener)? That seems like it would be more consistent with the net package.

CC @rsc

@gopherbot gopherbot added this to the Proposal milestone Sep 24, 2019
@gopherbot gopherbot added the Proposal label Sep 24, 2019
@slrz

This comment has been minimized.

Copy link

@slrz slrz commented Sep 25, 2019

How would this differ from net.Pipe?

@iangudger

This comment has been minimized.

Copy link
Contributor Author

@iangudger iangudger commented Sep 25, 2019

Sorry, somehow I missed that.

The part that is still missing through is a net.Listener to go along with it.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Sep 25, 2019

It would be buffered, which net.Pipe is not.
Perhaps net.NewPipeBuffer would be enough,
instead of all those extra functions.

@iangudger

This comment has been minimized.

Copy link
Contributor Author

@iangudger iangudger commented Sep 25, 2019

A buffered solution would be useful.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 2, 2019

Right now we have in package net:

func Pipe() (Conn, Conn)

It sounds like the minimal addition here to solve the problem would be

func BufferedPipe() (Conn, Conn)

I believe packages could build arbitrary other things easily on top of that.

Do people agree that this is the right path forward?

@cristaloleg

This comment has been minimized.

Copy link

@cristaloleg cristaloleg commented Oct 4, 2019

Yes, please, buffered pipe will be very useful.

@jared2501

This comment has been minimized.

Copy link

@jared2501 jared2501 commented Oct 5, 2019

That would be great! Especially useful for tests that exercise code using the tls package, which deadlocks if using net.Pipe.

@rsc rsc changed the title proposal: net: in-memory transport proposal: net: add BufferedPipe (buffered Pipe) Oct 9, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 9, 2019

OK, so it sounds like people agree that adding net.BufferedPipe (as suggested in #34502 (comment)) will resolve this.
As retitled, sounds like a likely accept.

Leaving open for a week for final comments.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 16, 2019

Accepted.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 16, 2019

Somehow the discussion of this proposal failed to reference the existing discussion in #28790 and #24205. In last week's proposal review notes it was mentioned as “retitled” but the new title (and the key “buffered” detail) was omitted from the notes.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 16, 2019

As noted in #28790 (comment), I'm not sure that there is any reasonable universal notion of “buffered” to be applied here.

  • Is the buffer of a bounded size, or unbounded?
  • If bounded:
    • How many writes or bytes will be buffered, and why?
    • If the the bound is unspecified, how will we discourage user code from making assumptions about it?
    • How will we help users to detect deadlocks masked by variable-sized writes and buffering?
@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented Oct 16, 2019

As the previous proposal meeting notes did not convey the correct title, I'm moving this back into the final comment period to allow for further discussion.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 16, 2019

If unbounded, how will the reader and writer avoid flow-control issues, such as the one described in #32840 (comment)?

Without flow control, it is remarkably easy to accidentally write a program whose memory footprint depends on what should be implementation details of the Go scheduler.

@iangudger

This comment has been minimized.

Copy link
Contributor Author

@iangudger iangudger commented Oct 16, 2019

I was thinking the behavior could be modeled after Unix sockets. Unix sockets currently work well for the use cases described, but they are not cross platform. I was thinking that the code for this new buffered pipe could be modeled after the Unix socket code in gVisor, which is a fairly complete and CLA compliant Unix socket emulator. It supports a lot of extra features we don't need here, so it likely wouldn't be a good idea to use the code directly.

In my experience, it does not seem to be common for applications to rely on a specific socket buffer size. gVisor does not use the same default buffer sizes as Linux and this hasn't really been a problem so far. Many applications depend on there being a maximum buffer size, so it probably shouldn't be unbounded.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 16, 2019

Many applications depend on there being a maximum buffer size, so it probably shouldn't be unbounded.

I'm more concerned about applications depending on the minimum buffer size than the maximum. (Otherwise, zero is a perfectly valid buffer size — so why would we need to add an implementation with a nonzero buffer size?)

@iangudger

This comment has been minimized.

Copy link
Contributor Author

@iangudger iangudger commented Oct 16, 2019

It seems that for Unix sockets, AMD64 Linux uses a default buffer size of 208 kB and IA32 Linux uses 160 kB. gVisor has been using 16 kB largely without issue.

The default send buffer size for TCP sockets is documented as 16 kB. That may be were gVisor's default came from.

I propose 16 kB.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 16, 2019

Can you give some examples of Go programs that require a buffer, but are correct and non-racy with a fixed buffer of size ≤ 16kB?

IIRC, the examples discussed in #28790 had fairly wide variations in buffering behavior. (See specifically #28790 (comment).)

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 16, 2019

See also #32990, in which a test using an unbuffered pipe exposed an implicit / unintended dependency on buffer sizes, which would have otherwise gone undetected.

@iangudger

This comment has been minimized.

Copy link
Contributor Author

@iangudger iangudger commented Oct 17, 2019

Well, I thought that I needed it for something, but it turns out that the unbuffered version works just fine.

Tests for the tls package was mentioned as a use case. Does anyone else need a buffered net.Conn pipe?

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Oct 23, 2019

@bcmills What is your concrete suggestion? Do you propose that BufferedPipe (#34502 (comment)) should take a size argument?

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 23, 2019

My concrete suggestion is that we not add a buffered pipe to the standard library at all.

The details of buffering do not seem to generalize well, and an unbuffered pipe suffices for most use-cases — and exposes more hidden assumptions in the process.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Oct 23, 2019

@iangudger what was the use case for your original proposal, re "connect two Go libraries which use the net interfaces"?

@as

This comment has been minimized.

Copy link
Contributor

@as as commented Oct 23, 2019

The previous proposal failed to demonstrate that a buffered pipe was impossible to implement by composing a regular pipe with a buffer. #28790 (comment)

This proposal seems to expose an arbitrary buffered pipe whose buffering properties are unknown.

Questions:

  • What is the usefulness of having a buffering function that the user can't tune?
  • What is the buffer's size?
  • Does the buffered pipe preserve message delimiters? (i.e., does one write to the write end map to one read on the read end with the same data)
@iangudger

This comment has been minimized.

Copy link
Contributor Author

@iangudger iangudger commented Oct 23, 2019

What is the usefulness of having a buffering function that the user can't tune?

Quite a bit actually. Most allocations don't tune the buffers of the sockets they create and the defaults can vary from system to system. Many applications just depend on having at least a small buffer.

What is the buffer's size?

I suggested 16 kB.

Does the buffered pipe preserve message delimiters?

Both are useful. I would support both as options.

What about x/net instead of net? Then, if it is popular, it would have the option of graduating at a later date.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Oct 23, 2019

@iangudger you overlooked my Q above...

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 23, 2019

@iangudger, why x/net at all? There are myriad buffered-pipe implementations outside of the standard library already (with varying buffering characteristics).

If you believe that one of them is (or can be made) sufficiently general with a sufficiently simple API, nothing is stopping you from tracking its popularity today.

(See also #17244.)

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 30, 2019

There were lots of deadlocks in crypto/tls using the unbuffered pipe that did not happen in the real network connections. Were they worth chasing down? It's unclear to me. It would be nice to be able to simulate something closer to a real network connection, where there is a limited buffer but the limit is definitely not 0.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 31, 2019

There were lots of deadlocks in crypto/tls using the unbuffered pipe that did not happen in the real network connections.

There are several apparent type-state issues remaining in crypo/tls and net/http.¹ It is not obvious to me that none of those are bugs typically masked by buffering (and exposed only under real-but-atypical conditions).

It would be nice to be able to simulate something closer to a real network connection, where there is a limited buffer but the limit is definitely not 0.

If so, then we're back to needing to resolve the questions about specific buffer sizes, and whether the buffer should apply to a number of bytes, a number of writes, or something else entirely.

Any specific buffer size introduces a test-fidelity problem: if crypto/tls assumes (and is only tested with) some arbitrary nonzero buffer size, how do we ensure that it actually has that minimum buffer size when it is using a real connection? And is an arbitrary size really universal enough to commit to in an exported, Go-1-compatible API?

The obvious endpoints for buffering in a test are “whatever a real network connection does” and “the absolute minimum possible amount of buffering”.

To test “whatever a real network connection does”, we should arguably test with a real network connection instead of an in-process one: odds are good that “something closer to a real network connection” would still not be close enough to catch edge-cases that occur in practice, and testing with a real connection also improves test coverage for the real implementation as a side-effect.

To test “the absolute minimum possible amount of buffering”, we can today test with net.Pipe, because no buffering at all is certainly the minimum possible.

The need for some third point in the middle is not at all clear to me, and the lack of concrete examples in response to my previous question (#34502 (comment)) seems telling.


¹ Some example issues that may relate to buffering

@fraenkel

This comment has been minimized.

Copy link
Contributor

@fraenkel fraenkel commented Oct 31, 2019

Additional ones in http2:
x/net/http2: #33425 #32388

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 6, 2019

@iangudger, any responses to @bcmills's latest message?

@rsc rsc added this to Active in Proposals Nov 27, 2019
@rsc rsc added the Proposal-Hold label Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

Putting this on hold for response from @iangudger or else me digging up the old examples of crypto/tls code that would have been fixed by buffering. @bcmills and I look at this about once a week and keep postponing.

@rsc rsc moved this from Active to Hold in Proposals Dec 4, 2019
@navytux

This comment has been minimized.

Copy link
Contributor

@navytux navytux commented Dec 19, 2019

By analogy with channels pipe's buffer can be a property that is configured at the time when the pipe is created. By analogy with channels the following cases are principally distinct: a) bufsize=0 - provides synchronization in between read/write, b) bufsize=1+something - provides way for sender not to be blocked if there is no matching receiver yet, at least for 1 send. This property can be explicitly used to avoid deadlocks. By the same analogy with channels, the amount of buffering, and/or other options, could be represented by options passed into pipe factory function, e.g.

r, w  = io.Pipe2(io.PipeOptions{BufferSize: ..., OtherOption: ...})

By using PipeOptions options can be extended in the future without introducing other API calls like io.BufferedAndSomethingElsePipe etc. For naming instead of io.Pipe2 e.g. io.PipeWithOptions or something similar could be used.

Re: If one wants to connect two Go libraries which use the net interfaces, the only cross platform way to do it with the standard library is to use the loopback:

There is pipenet package that I did some time ago for in-process testing of blocks that use net interfaces.

Please forgive me if my reply is not very useful. I found this issue randomly while looking for io.Pipe whose read/write can be canceled (e.g. accept context or something similar).

Kirill

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.