ioutil.ReadAll()

[Patrik Iselind]

Hi,

I have a hard time imagining when https://golang.org/pkg/io/ioutil/#ReadAll would produce err != nil. The documentation doesn't tell either. It would help when writing unittests for code that use ioutil.ReadAll().

// Patrik

[Axel Wagner]

For example:

https://play.golang.org/p/ycoR-jU48J9

The only read error it ignores is io.EOF (which makes complete sense, when you think about it), everything else get passed up.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[peterGo]

Patrik,

Someone pulled the floppy disk out of the FDD. Someone waved a magnet over the HDD, Cosmic rays hit the SSD, Someone pulled Eternet cable out of its socket. And so on.

How about a simple bug? "ioutil.ReadAll: invalid argument": https://play.golang.org/p/r8mfn5KvaE8

Peter

[Patrik Iselind]

What can go wrong if i try to use ioutil.ReadAll() on a HTTP response? Isn't that very hard to fake?

[Axel Wagner]

Not very, but it does depend on the details. For example, you might provide your own http.Transport for the client to use, or even your own net.Conn. You might have the server stop sending any data, so eventually the connection will timeout.

The question is, though, why would you want that? Is that actually a path that is worth testing? Personally, I kind of doubt it. You'll probably get more bang for your buck, if you instead send back broken/incomplete data from the server and see if the client handles that correctly.

On Tue, Feb 13, 2018 at 5:01 PM, Patrik Iselind <patri...@gmail.com> wrote:

What can go wrong if i try to use ioutil.ReadAll() on a HTTP response? Isn't that very hard to fake?

--

[mrx]

On Wed, Feb 14, 2018 at 10:30 AM, Axel Wagner <axel.wa...@googlemail.com> wrote:

On Wed, Feb 14, 2018 at 10:01 AM mrx <patri...@gmail.com> wrote:

On Tue, Feb 13, 2018 at 5:26 PM, Axel Wagner <axel.wa...@googlemail.com> wrote:

Not very, but it does depend on the details. For example, you might provide your own http.Transport for the client to use, or even your own net.Conn.

Using ioutils.ReadAll() on a HTTP request means to me to read out the response's body. I cannot see how http.Transport and net.Conn would have anything to do with this.

Presumably, to read out a response body means, that you made a request. By passing in a custom http.Transport, you can have that transport return a non-EOF error at will in tests.

It would be far more helpful, if there would be actual code here.

Sure thing, taken from the http docs:

resp, err := http.Get("http://example.com/")
if err != nil {
	// handle error
}
defer resp.Body.Close()
body, err := ioutil.ReadAll(resp.Body)

 

That err returned from ReadAll. I cannot see how that can possibly fail.

You might have the server stop sending any data, so eventually the connection will timeout.

So what you're saying is that unless the response contain chunked data, ioutil.ReadAll() will never fail?

I say that ioutil.ReadAll will fail if and only if the io.Reader it's called with returns a non-EOF, non-nil error on some read. That, at least, seems the most obvious semantics of that function to me and it's how I read its documentation:

 

> ReadAll reads from r until an error or EOF and returns the data it read. A successful call returns err == nil, not err == EOF.

Sure, that make sense. When testing for err i try to imagine what could go wrong, to get the error handling correct. But in this case (code from http docs above) i cannot see it...

 

The question is, though, why would you want that?

As ioutil.ReadAll does return an error in its signature, i think it's good form to test it. Don't you?

No, I don't think I do, actually :) Like, what failure modes is this testing, how often will they occur, how likely is it, that a future change would break the behavior? I'm just having trouble coming up with a bug that might be introduced in the future, that could be caught by testing this.

Checking every branch seems to be an incarnation of the "we need 100% line coverage" testing philosophy, but honestly, I don't believe that's particularly helpful, because a) path coverage is much more important than line coverage

I'm not after a 100% line coverage. =)

Is there a way to get branch/path coverage in Go? I cannot find anything like that in the help from go test.

 

and b) 100% is unattainable in practice. Adhering to some kind of mechanical rule about what code must have test coverage will just lead to wasted time - if there are zero future bugs with or without that test, it definitionally was useless. And I'd say that the likelihood that this is going to catch a future bug is very low. So if, with very high probability, writing that test is a waste of time then that seems a good enough reason not to write it.

I agree, i'm just trying to get the error handling correct. That's really difficult if i cannot even imagine what could go wrong.

 

(unless, of course, you are trying to write tests for ReadAll itself, in which case checking the whole API contract makes of course sense. But I assume you're not :) )

You're correct, i'm not writing tests for ioutil.ReadAll()

 

Is that actually a path that is worth testing? Personally, I kind of doubt it.

That's kind of it really, i am having a hard time making up my mind here. That's why i come for the golang nuts wisdom.
 

You'll probably get more bang for your buck, if you instead send back broken/incomplete data from the server and see if the client handles that correctly.

I already test for this kind of problems in my unittests. It's more a matter of what to do with the error return value from ioutil.ReadAll() when i cannot see how i could ever get anything but err == nil. It might just be me, that doesn't know enough about the ioutil.ReadAll() internals.

After digging some into the code, it does inform a sensible additional test you might be doing, as it also returns an error if the reader returns too much data - so it would make sense to check what happens if the server would send back, say, an infinite bytestream. But that still isn't really a test for the ReadAll error return, as a check for "does this function behave well if fed unreasonable data".

And to answer the question of what to do about err != nil: You should handle it :) It means the data the server sent was broken in some way, you should handle that in some way that is appropriate for the application you are writing - which might include logging it, ignoring it, aborting, retrying or anything else, really :) Presumably, you are also handling invalid data somehow - this shouldn't be any different.

Handling those errors still mean that i need to have at least an idea or two on what could happen, no?

// Patrik

[Jakob Borg]

On 14 Feb 2018, at 10:50, mrx <patri...@gmail.com> wrote:

resp, err := http.Get("http://example.com/")
if err != nil {
	// handle error
}
defer resp.Body.Close()
body, err := ioutil.ReadAll(resp.Body)

 

That err returned from ReadAll. I cannot see how that can possibly fail.

You need a more vivid imagination. The server might crash, the network might fail, a firewall in between might reboot, and so on. Any issue along the path might result in a timeout, short read (server said “content-lenght: 42” only sent five bytes), or other errors. 

//jb

[Axel Wagner]

Why are the things mentioned so far not sufficient? ReadAll returns a non-nil error, if the reader you are passing to it returns a non-nil, non-EOF error. In this case, you are passing Response.Body, which does not make any guarantees regarding the errors it returns, so you should just treat the error returned as opaque (you can't reliably check the error conditions exhaustively anyway). Things that might go wrong include (but are not limited to) anything that might go wrong when reading from a network connection, any error resulting from a broken encoded stream that the client transparently decodes (like gzip encoding, TLS…)…

[Axel Wagner]

On Wed, Feb 14, 2018 at 10:01 AM mrx <patri...@gmail.com> wrote:

On Tue, Feb 13, 2018 at 5:26 PM, Axel Wagner <axel.wa...@googlemail.com> wrote:

Not very, but it does depend on the details. For example, you might provide your own http.Transport for the client to use, or even your own net.Conn.

Using ioutils.ReadAll() on a HTTP request means to me to read out the response's body. I cannot see how http.Transport and net.Conn would have anything to do with this.

Presumably, to read out a response body means, that you made a request. By passing in a custom http.Transport, you can have that transport return a non-EOF error at will in tests.

It would be far more helpful, if there would be actual code here.

You might have the server stop sending any data, so eventually the connection will timeout.

So what you're saying is that unless the response contain chunked data, ioutil.ReadAll() will never fail?

I say that ioutil.ReadAll will fail if and only if the io.Reader it's called with returns a non-EOF, non-nil error on some read. That, at least, seems the most obvious semantics of that function to me and it's how I read its documentation:

 

> ReadAll reads from r until an error or EOF and returns the data it read. A successful call returns err == nil, not err == EOF.

The question is, though, why would you want that?

As ioutil.ReadAll does return an error in its signature, i think it's good form to test it. Don't you?

No, I don't think I do, actually :) Like, what failure modes is this testing, how often will they occur, how likely is it, that a future change would break the behavior? I'm just having trouble coming up with a bug that might be introduced in the future, that could be caught by testing this.

Checking every branch seems to be an incarnation of the "we need 100% line coverage" testing philosophy, but honestly, I don't believe that's particularly helpful, because a) path coverage is much more important than line coverage and b) 100% is unattainable in practice. Adhering to some kind of mechanical rule about what code must have test coverage will just lead to wasted time - if there are zero future bugs with or without that test, it definitionally was useless. And I'd say that the likelihood that this is going to catch a future bug is very low. So if, with very high probability, writing that test is a waste of time then that seems a good enough reason not to write it.

(unless, of course, you are trying to write tests for ReadAll itself, in which case checking the whole API contract makes of course sense. But I assume you're not :) )

Is that actually a path that is worth testing? Personally, I kind of doubt it.

That's kind of it really, i am having a hard time making up my mind here. That's why i come for the golang nuts wisdom.
 

You'll probably get more bang for your buck, if you instead send back broken/incomplete data from the server and see if the client handles that correctly.

I already test for this kind of problems in my unittests. It's more a matter of what to do with the error return value from ioutil.ReadAll() when i cannot see how i could ever get anything but err == nil. It might just be me, that doesn't know enough about the ioutil.ReadAll() internals.

[mrx]

On Tue, Feb 13, 2018 at 5:26 PM, Axel Wagner <axel.wa...@googlemail.com> wrote:

Not very, but it does depend on the details. For example, you might provide your own http.Transport for the client to use, or even your own net.Conn.

Using ioutils.ReadAll() on a HTTP request means to me to read out the response's body. I cannot see how http.Transport and net.Conn would have anything to do with this.
 

You might have the server stop sending any data, so eventually the connection will timeout.

So what you're saying is that unless the response contain chunked data, ioutil.ReadAll() will never fail?
 

The question is, though, why would you want that?

As ioutil.ReadAll does return an error in its signature, i think it's good form to test it. Don't you?

 

Is that actually a path that is worth testing? Personally, I kind of doubt it.

That's kind of it really, i am having a hard time making up my mind here. That's why i come for the golang nuts wisdom.
 

You'll probably get more bang for your buck, if you instead send back broken/incomplete data from the server and see if the client handles that correctly.

I already test for this kind of problems in my unittests. It's more a matter of what to do with the error return value from ioutil.ReadAll() when i cannot see how i could ever get anything but err == nil. It might just be me, that doesn't know enough about the ioutil.ReadAll() internals.

// Patrik

[matthe...@gmail.com]

The source for ioutil.ReadAll() shows that it can return a bytes.ErrTooLarge or an error from the io.Reader.

So what you're saying is that unless the response contain chunked data, ioutil.ReadAll() will never fail?

I thought typically the http response isn’t buffered into memory first, so the io.Reader is reading directly from the network which is error prone.

A way to test would be to cause the server to panic while sending the body to the client running ReadAll or just Read. Another would be to have the body be larger than virtual memory available to the client process.

Matt

[Patrik Iselind]

Den 2018-02-14 kl. 11:42, skrev Axel Wagner:
> Why are the things mentioned so far not sufficient?

The presented reasons are enough. I was just seeing things the wrong way
and it took quite a while to get my head on straight. After some
consideration i have repaired my views and everything is nice and shiny
again. Thanks a lot for your patience.

Thank you all for your input.

// Patrik

[ANKIT KUMAR]

resp, err := http.Get("http://example.com/") if err != nil { // handle error } defer resp.Body.Close() body, err := ioutil.ReadAll(resp.Body)

 

That err returned from ReadAll.

how to sloved