{"action":"created","issue":{"url":"https://api.github.com/repos/golang/go/issues/28790","repository_url":"https://api.github.com/repos/golang/go","labels_url":"https://api.github.com/repos/golang/go/issues/28790/labels{/name}","comments_url":"https://api.github.com/repos/golang/go/issues/28790/comments","events_url":"https://api.github.com/repos/golang/go/issues/28790/events","html_url":"https://github.com/golang/go/issues/28790","id":380633140,"node_id":"MDU6SXNzdWUzODA2MzMxNDA=","number":28790,"title":"Proposal: io: add a buffered pipe","user":{"login":"mvdan","id":3576549,"node_id":"MDQ6VXNlcjM1NzY1NDk=","avatar_url":"https://avatars0.githubusercontent.com/u/3576549?v=4","gravatar_id":"","url":"https://api.github.com/users/mvdan","html_url":"https://github.com/mvdan","followers_url":"https://api.github.com/users/mvdan/followers","following_url":"https://api.github.com/users/mvdan/following{/other_user}","gists_url":"https://api.github.com/users/mvdan/gists{/gist_id}","starred_url":"https://api.github.com/users/mvdan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mvdan/subscriptions","organizations_url":"https://api.github.com/users/mvdan/orgs","repos_url":"https://api.github.com/users/mvdan/repos","events_url":"https://api.github.com/users/mvdan/events{/privacy}","received_events_url":"https://api.github.com/users/mvdan/received_events","type":"User","site_admin":false},"labels":[{"id":236419512,"node_id":"MDU6TGFiZWwyMzY0MTk1MTI=","url":"https://api.github.com/repos/golang/go/labels/Proposal","name":"Proposal","color":"ededed","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":{"url":"https://api.github.com/repos/golang/go/milestones/30","html_url":"https://github.com/golang/go/milestone/30","labels_url":"https://api.github.com/repos/golang/go/milestones/30/labels","id":1373555,"node_id":"MDk6TWlsZXN0b25lMTM3MzU1NQ==","number":30,"title":"Proposal","description":"Proposals that are pending (not yet accepted/rejected).\r\n","creator":{"login":"rsc","id":104030,"node_id":"MDQ6VXNlcjEwNDAzMA==","avatar_url":"https://avatars1.githubusercontent.com/u/104030?v=4","gravatar_id":"","url":"https://api.github.com/users/rsc","html_url":"https://github.com/rsc","followers_url":"https://api.github.com/users/rsc/followers","following_url":"https://api.github.com/users/rsc/following{/other_user}","gists_url":"https://api.github.com/users/rsc/gists{/gist_id}","starred_url":"https://api.github.com/users/rsc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rsc/subscriptions","organizations_url":"https://api.github.com/users/rsc/orgs","repos_url":"https://api.github.com/users/rsc/repos","events_url":"https://api.github.com/users/rsc/events{/privacy}","received_events_url":"https://api.github.com/users/rsc/received_events","type":"User","site_admin":false},"open_issues":237,"closed_issues":684,"state":"open","created_at":"2015-10-24T01:40:49Z","updated_at":"2018-11-14T10:30:54Z","due_on":null,"closed_at":null},"comments":9,"created_at":"2018-11-14T10:30:53Z","updated_at":"2018-11-14T18:46:00Z","closed_at":null,"author_association":"MEMBER","body":"#### Summary\r\n\r\nAdd a way to easily create a buffered `io.Pipe`. Currently, one has to write their own from scratch. This leads to dozens of separate implementations in the wild, and likely many correctness issues and hidden bugs.\r\n\r\n#### Description\r\n\r\n`io.Pipe` is synchronous, as per https://golang.org/pkg/io/#Pipe:\r\n\r\n> Reads and Writes on the pipe are matched one to one except when multiple Reads are needed to consume a single Write. That is, each Write to the PipeWriter blocks until it has satisfied one or more Reads from the PipeReader that fully consume the written data. The data is copied directly from the Write to the corresponding Read (or Reads); there is no internal buffering.\r\n\r\nThis is fine for most use cases. However, a buffered pipe can still be useful or necessary in many situations. Below is a list of third-party implementations I was able to find after a ten-minute search:\r\n\r\n* https://github.com/golang/go/blob/0436b162397018c45068b47ca1b5924a3eafdee0/src/net/net_fake.go#L173\r\n* https://github.com/golang/go/blob/f58b02a29c395b5ec28bb548b1fd3ca18e2b3a4f/src/net/http/h2_bundle.go#L3688\r\n* https://github.com/golang/crypto/blob/a49355c7e3f8fe157a85be2f77e6e269a0f89602/ssh/handshake_test.go#L42\r\n* https://github.com/moby/vpnkit/blob/237a0c3dfd57753f798a4cb4143cb1ed2ee118b9/go/pkg/libproxy/loopbackconn.go#L19\r\n* https://github.com/vmware/vic/blob/68eef3661c332fad0fdf71b99d4a10bda06ee1e4/lib/install/vchlog/buffered_pipe.go\r\n* https://github.com/v2ray/v2ray-core/blob/891bdd14fe7cbb99b3d64894a633e83cd2691b72/transport/pipe/impl.go\r\n* https://github.com/rogpeppe/rog-go/blob/f57ad5e24ab7813942c045f86eda1066eb969e98/loopback/loopback.go#L107\r\n* https://github.com/vanadium/go.lib/blob/d0dec8edd1bfe451a41763608a8518f1dba8e7ae/gosh/buffered_pipe.go\r\n* https://github.com/karalabe/bufioprop/blob/98d36783a81dc759efaef68b4ad9a7d3bfa4090b/pipe.go\r\n* https://github.com/kr/spdy/blob/b6b3b6ca43a79eafe2fb96a44066e27768457928/spdyframing/pipe.go#L7\r\n* https://github.com/whyrusleeping/bufpipe\r\n\r\nNote the first two, which live in the standard library itself. They're also interesting, because `net`'s requires deadlines, and `http`'s supports any underlying pipe buffer implementation.\r\n\r\nThe main point of the proposal is that there are dozens of well-used buffered pipe implementations out there. This is particularly worrying, because writing a correct one isn't easy. It's very likely that some of the implementations out there have subtle bugs, such as misusing `sync.Cond` or being racy when many reads and writes happen concurrently.\r\n\r\nI'm raising this proposal precisely because I had written my own buggy buffered pipe, which made `go test -race` find data races about 5% of the time. I \"fixed\" that by rewriting my tests to work around the lack of buffering, and using `io.Pipe` instead: https://github.com/mvdan/sh/commit/5ffc4d94f4d4e57cbb02063768b67c1b337bf9cc\r\n\r\nGranted, my code being incorrect doesn't mean everyone's code is too. But it does help illustrate the point; writing a buffered pipe seems easy, and the lack of an implementation in the standard library seems to encourage that people write their own, likely buggy implementation.\r\n\r\nThis has been discussed in golang-dev before, although no common implementation was found or proposed: https://groups.google.com/forum/#!topic/golang-dev/k0bSal8eDyE\r\n\r\nThere's also a proposal to make `net.Pipe` asynchronous in Go2; https://github.com/golang/go/issues/24205. That proposal is somewhat related to this one, since `net.Pipe` currently uses `io.Pipe` under the hood. This could however be changed in Go2.\r\n\r\n#### Proposed API\r\n\r\nThe simplest change would be to add something like:\r\n\r\n```\r\n// BufferedPipe creates an asynchronous in-memory pipe, internally buffering up\r\n// to n bytes.\r\nfunc BufferedPipe(size int) (*PipeReader, *PipeWriter)\r\n```\r\n\r\nIdeally `PipeReader` and `PipeWriter` would have been interface types from the start. Within Go1, we can work around that by having the `PipeWriter` struct simply embed the actual pipe implementation, be it synchronous or asynchronous.\r\n\r\nAnother possible path in Go1 is to go the http2 route, allowing any underlying pipe buffer implementation. I personally think this would be a bit too niche for the `io` package, but perhaps @bradfitz has opinions on that. Aside from http2, all buffered pipe packages seem to build on the current `io.Pipe` semantics.\r\n\r\nIf the `net` package needs read and write deadlines, they could always be added to the reader and writer returned by the constructors. This should be a backwards compatible change, and could be discussed as a separate proposal.\r\n\r\nFinally, if we were to consider this as a Go2 change, the cleaned up version of my first proposed change would be:\r\n\r\n```\r\ntype PipeReader interface {\r\n\tio.Reader\r\n\tio.Closer\r\n\tCloseWithError(err error) error\r\n}\r\n\r\ntype PipeWriter interface {\r\n\tio.Writer\r\n\tio.Closer\r\n\tCloseWithError(err error) error\r\n}\r\n\r\nfunc Pipe() (PipeReader, PipeWriter)\r\nfunc BufferedPipe(size int) (PipeReader, PipeWriter)\r\n```\r\n\r\nWe could even go one step further and mirror `make(chan T, size)`, only having `func Pipe(size int)` where `Pipe(0)` would return a synchronous/unbuffered pipe.\r\n\r\nThe `net` package could either mirror these proposed Go2 changes, or break with the connection to the `io` package entirely.\r\n\r\n/cc @bcmills @dsnet @bradfitz @rogpeppe (and @neelance for `net_fake.go`, and @djs55 for vpnkit's `loopbackconn.go`)"},"comment":{"url":"https://api.github.com/repos/golang/go/issues/comments/438773590","html_url":"https://github.com/golang/go/issues/28790#issuecomment-438773590","issue_url":"https://api.github.com/repos/golang/go/issues/28790","id":438773590,"node_id":"MDEyOklzc3VlQ29tbWVudDQzODc3MzU5MA==","user":{"login":"mvdan","id":3576549,"node_id":"MDQ6VXNlcjM1NzY1NDk=","avatar_url":"https://avatars0.githubusercontent.com/u/3576549?v=4","gravatar_id":"","url":"https://api.github.com/users/mvdan","html_url":"https://github.com/mvdan","followers_url":"https://api.github.com/users/mvdan/followers","following_url":"https://api.github.com/users/mvdan/following{/other_user}","gists_url":"https://api.github.com/users/mvdan/gists{/gist_id}","starred_url":"https://api.github.com/users/mvdan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mvdan/subscriptions","organizations_url":"https://api.github.com/users/mvdan/orgs","repos_url":"https://api.github.com/users/mvdan/repos","events_url":"https://api.github.com/users/mvdan/events{/privacy}","received_events_url":"https://api.github.com/users/mvdan/received_events","type":"User","site_admin":false},"created_at":"2018-11-14T18:46:00Z","updated_at":"2018-11-14T18:46:00Z","author_association":"MEMBER","body":"I think the \"buffered channel\" analogy only goes as far as the name and the high-level idea. It breaks quickly, because a buffered channel always passes messages intact, while an `io.Pipe` may break chunks of writes into smaller pieces. For example, one pipe write of 100 bytes can be unblocked by four reads of 20 bytes.\r\n\r\nSimilarly, a buffered pipe could mean that two pipe writes of 50 bytes end up as a single pipe read of 100 bytes, joining the two sent chunks of bytes.\r\n\r\nWe can agree that `io.Pipe` is \"synchronous\". I don't think mutually excluded data maps to reads and writes, because we're not talking about messages that remain as themselves - chunks of bytes may be split or joined, as explained above.\r\n\r\nAs for “an upper bound on the total number of elements sent”, I think the equivalent in a buffered pipe would be \"the maximum size of one message sent on the pipe\", where one message could consist of many writes. In practice, programs tend to know the maximum message size, so they can use that knowledge to guarantee that they can write a message without blocking. (thanks to @rogpeppe for helping word this paragraph clearly)\r\n\r\nAs for practical examples - net_fake uses `newBufferedPipe(65536)`, which I assume comes from the maximum size of a TCP packet."}}
{"actor":{"display_login":"mvdan"}}