io: make MultiReader documentation more explicit #7216
Labels
Milestone
Comments
I don't understand what you've said. Could you wirte a small program what do you think MultiReader is buggy? > is MultiReader.Read supposed to stop after the first Reader returns EOF? Yes, (*multireader).Read will return after the first Reader returns EOF, however, it won't return EOF itself, so I don't think you can call that "stop" as that signals the caller that it can always call Read again. Status changed to WaitingForReply. |
The bug tracker is not for ranting. There are no implementation issues here, and your P.S. is wrong. A simple grep finds other uses. And many uses in outside projects (e.g. Camlistore has 12) If you have specific documentation suggestions, discuss first on golang-nuts and see if others agree. If so, then file a bug. We've had many documentation issues in the past, but I don't agree this is a major problem. The docs seem accurate and clear to me, at least. Status changed to WorkingAsIntended. |
@ryguillian I can't tell if you're being sarcastic. Are you actually going to give us a clear description of the buggy behavior? Or an example program? I've used MultiReader a handful of times and it's always behaved the way I expect, but I'm willing to believe there's an edge case lurking somewhere. |
I think this issue originate from misunderstanding of how io.Reader works. (e.g. Read is allowed to return 0, nil, and it's treated as a no-op, and it's also allowed to return less than len(buf) and non-nil error, etc.) I also believe the MultiReader implementation is as correct as it can be. |
When you call Read on MultiReader with a slice larger than what's left in the rest of the file MultiReader yields rather than trying to keep reading. As pointed out, this is more of a pathological design in Go. It's like a tacit "yield" using n < len(slice) && err==nil as a "more to read" signal. If the Go team thinks this is okay, fine. I would be interested in the rationale for this pervasive design. |
There is always more to read until you get io.EOF or another error. http://golang.org/pkg/io/#Reader |
Also there is no "yield" in Go. It would be clearer if you would use the same terminology as the language spec where possible. It is common for a Read to return n < len(slice). This is by design, so that many wrapped readers can share the same buffer and avoid copying. It has worked very well so far. I think you are just misunderstanding the io.Reader design. |
Alright this is getting obnoxious. I wasn't trying to be sarcastic or a pain in the neck, I was honestly confused. I'm not an idiot. I know Go doesn't have a 'yield' statement. io.MultiReader.Read returns in the middle of a for loop once it receives io.EOF from an underlying reader. Fine. I get it. I know how it works. _As I said_ this is _obviously_ a _design decision_ that's _pervasive_ in Go. Okay? Here's what io.Reader's documentation that a...@golang.org linked to: 'If some data is available but not len(p) bytes, Read conventionally returns what is available instead of waiting for more.' What does 'available' mean? You have to be joking if you tell me this is clear. Also, it says 'conventionally'. MultiReader is already inside of a for loop. Also, why is the data from the next reader in mr.readers 'not available'? Can anybody actually explain the actually design philosophy behind this or is it just made-up and to be intrepidly defended because that's what we say and that's what goes? If that's the attitude at Go, fine, but then count you user base one less. |
I did not intend to be obnoxious. I was just asking you to be precise (just as you are asking of us), so that everyone is clear on what is being discussed. As for io.Reade, what "available" means depends on the context. io.Reader is a general interface implemented by many different things. For instance, a Read from a network connection might just return whatever data has appeared on the wire (as handed to the Go program from the kernel by a read syscall). But then another io.Reader implementation might decide to wait until the Read buffer is full before returning (this is unusual, and as such should be documented by the implementor, hence the "conventionally" in the io.Reader docs). Both are valid implementations of io.Reader. But as a user of io.Reader, you must always repeatedly call Read until you get io.EOF. I'm not seeing how this is "pathalogical" though. Take a look at the various io.Reader implementations in the tree to get an idea of how this works in practice. |
As andrew explained, we first made the decision in the design of io.Reader that it is allowed to not fill the buffer (btw, it's also the design of the read(2) syscall after which io.Reader is modeled). Given that, MultiReader's behavior is perfectly fine. If you want to read the buffer full, use io.ReadFull, which saves a lot of trouble. And by looking at the source of io.ReadAtLeast you will find that actually filling the buffer is difficult that what you think. Duplicate all that logic in MultiReader doesn't make sense given ReadFull already implements it. Honestly, do you want to duplicate that logic in every io.Reader implementation you write? Try take a look at the compress/* Readers and see why it's not feasible to always fill the buffer passed in. |
The documentation describes the "what." If you want to implement or use an io.Reader, the docs explain how it works. You're asking *why* the io.Reader is so designed. We're suggesting that you take a look at the various implementations of io.Reader to get a feel for the various possibilities that the interface's specification provides. FWIW, you have received a somewhat chilly response in this bug because you launched an all out "I think you guys should change this," before actually understanding the system. The issue is already closed. |
The original report contains some useful feedback: the terminology of "drained" is not a term of art. Instead we should say "when all the provided Readers have been read to EOF," or similar. Labels changed: added documentation, release-go1.3. Owner changed to @adg. Status changed to Accepted. |
|
This issue was closed by revision 5be96aa. Status changed to Fixed. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
by ryguillian:
The text was updated successfully, but these errors were encountered: