What is the point of gzip Reader.Close?

[Steven Penny]

If I have a program like this, it fails as expected (on Windows):

~~~
package main

import (
   "fmt"
   "os"
)

func main() {
   old := "go1.16.5.src.tar.gz"
   os.Open(old)
   err := os.Rename(old, "new.tar.gz")
   // The process cannot access the file because it is being used by another process.
   fmt.Println(err)
}
~~~

However this program succeeds:

~~~
package main

import (
   "compress/gzip"
   "os"
)

func main() {
   old := "go1.16.5.src.tar.gz"
   f, err := os.Open(old)
   if err != nil {
      panic(err)
   }
   gzip.NewReader(f)
   f.Close()
   os.Rename(old, "new.tar.gz")
}
~~~

even though I did not close the gzip reader. My question is: what is the point
of closing the gzip reader? What "bad" can happen if you dont do it?

https://golang.org/pkg/compress/gzip#Reader.Close

[Axel Wagner]

There are two potential negative consequences I could imagine happening:

1. You don't get notified about checksum mismatches or the like. i.e. the data you read might be corrupted and you might not realize it.

2. There might, theoretically, be goroutines spawned which get leaked if you don't `Close` the `Reader`. I'd assume this is unlikely (I can't see a decompressor really benefiting from spawning goroutines) but it's possible.

Either way, it's also just good hygiene to *always* call `Close` on an `io.Closer`. If a library gives you a `Close` method, it usually expects you to call it and there's really no point in not doing it.

--
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...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/28eb98d4-c531-413d-b7f1-790b2600309fn%40googlegroups.com.

[Steven Penny]

Thanks for the response. Couple of points:

> 1. You don't get notified about checksum mismatches or the like. i.e. the
> data you read might be corrupted and you might not realize it.

I dont think this is true. I tried with this file [1], and Close returns the
same error as, for example io.Copy, so again Close seems redundant.

> Either way, it's also just good hygiene to *always* call `Close` on an
> `io.Closer`. If a library gives you a `Close` method, it usually expects you
> to call it and there's really no point in not doing it.

Even with something like net/http#Head? Seems pointless in that case too.

1. https://storage.googleapis.com/go-attachment/6550/0/will_hang.gz

[Axel Wagner]

To be clear, I said "things I could imagine". But:

On Mon, Jun 21, 2021 at 11:28 PM Steven Penny <srp...@gmail.com> wrote:

Thanks for the response. Couple of points:

> 1. You don't get notified about checksum mismatches or the like. i.e. the
> data you read might be corrupted and you might not realize it.

I dont think this is true. I tried with this file [1], and Close returns the
same error as, for example io.Copy, so again Close seems redundant.

That doesn't seem like a particularly conclusive test. There are a couple of layers of readers and I would assume some buffering is going on. So if there is indeed a checksum mismatch reported by `Close` but not a previous `Read`, it might depend on details like alignment of blocks or specific gzip parameters used in the compressed file. Saying "on this particular file, `Close` returned the same error" is just not meaningful.

That being said: Again, I was just guessing at potential issues. If I was sufficiently motivated, I would look at the code and see under what circumstances `Close()` would return an error - I tried, but it quickly became non-trivial enough for me to lose motivation.

> Either way, it's also just good hygiene to *always* call `Close` on an
> `io.Closer`. If a library gives you a `Close` method, it usually expects you
> to call it and there's really no point in not doing it.

Even with something like net/http#Head? Seems pointless in that case too.

Yes, even in that case and I'm not sure it is pointless. I would assume that the client code does not special case `HEAD` requests and just uses the same logic when returning the response as if it was a `GET` request - in which case you have to `Close` the body to make sure a Keep-Alive connection can be re-used (I think. Fuzzy on the details). Again, if I was sufficiently motivated, I would check.

But the docs seem very clear to me:

// The http Client and Transport guarantee that Body is always
// non-nil, even on responses without a body or responses with
// a zero-length body. It is the caller's responsibility to
// close Body. 

To me, this reads as clearly documenting that you have to close the body, even on responses without a body (like HEAD).

Again, the idiom is, if you get an `io.Closer`, `Close` should be called once you're done with it.

[Steven Penny]

> Again, the idiom is, if you get an `io.Closer`, `Close` should be called once
> you're done with it.

Thanks for the responses, but I am not convinced. Other than "its just good
practice", I havent seen a single concrete example where not closing a gzip
reader would cause a problem. Until that happens, I am just going to stop doing
it in my code.

As far as Im concerned, gzip Close seems about as useful as io.NopCloser. Again,
if I am wrong on this, I certainly want to know it, but at this moment it doesnt
seem that I am.

[Dan Kortschak]

The relevant code is in compress/flate. There you can see that it is
possible for the flate decompressor to advance without returning an
error, but storing the error in its err field. This is then returned
when the Close method is called. This particular case won't be seen
when a flate stream is read to completion, but if you partially read a
stream and encounter an error without closing you could see different
behaviours.

Dan

[Bruno Albuquerque]

Not to mention this all is an implementation detail and even if it did nothing today, relying on this behavior would be a recipe so see cobe breaking in the future. In this sense, the advice "if there is a Close() method, call it when you are done with it" is a very good one.

--
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...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/858087d9b1fe39c8e2949559b8677e21b1f8fbb1.camel%40kortschak.io.

[Steven Penny]

> Not to mention this all is an implementation detail and even if it did
> nothing today, relying on this behavior would be a recipe so see cobe
> breaking in the future. In this sense, the advice "if there is a Close()
> method, call it when you are done with it" is a very good one.

No other compress reader even has a Close method, so I think Im fine:

- https://golang.org/pkg/compress/bzip2
- https://golang.org/pkg/compress/flate
- https://golang.org/pkg/compress/lzw
- https://golang.org/pkg/compress/zlib

[Axel Wagner]

On Mon, Jun 21, 2021 at 11:53 PM Steven Penny <srp...@gmail.com> wrote:

Thanks for the responses, but I am not convinced. Other than "its just good
practice", I havent seen a single concrete example where not closing a gzip
reader would cause a problem.

This is what we call a black swan fallacy. 

 

Until that happens, I am just going to stop doing it in my code.

That's certainly your prerogative. Your code will be incorrect. But it's hardly our responsibility to stop you from willingly writing incorrect code.

[Bruno Albuquerque]

Dan just pointed out that the Close() method for the gzip reader actually does other things. If you are ok with not detecting a possible error with deferred reporting through the Close() method then, yes, you are ok. I see no good reason why you would though.

[Axel Wagner]

On Tue, Jun 22, 2021 at 12:31 AM Steven Penny <srp...@gmail.com> wrote:

No other compress reader even has a Close method, so I think Im fine:

- https://golang.org/pkg/compress/bzip2

This one hasn't.

 

- https://golang.org/pkg/compress/flate

- https://golang.org/pkg/compress/lzw
- https://golang.org/pkg/compress/zlib

All of these do.

 

--
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...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAP8dQmsfC6WW-UB6EZmGbFcpkAFODVr0uUMkGAkcuwZuYUuMJQ%40mail.gmail.com.

[Dan Kortschak]

On Mon, 2021-06-21 at 17:30 -0500, Steven Penny wrote:
> > No other compress reader even has a Close method, so I think Im
> > fine:
>
> - https://golang.org/pkg/compress/bzip2
> - https://golang.org/pkg/compress/flate
> - https://golang.org/pkg/compress/lzw
> - https://golang.org/pkg/compress/zlib

flate does, it returns an io.ReadCloser[1] and is the underlying
decompressor for gzip. lzw[2] and zlib[3] also returns an io.ReadCloser

[1]https://golang.org/pkg/compress/flate/#NewReader
[2]https://golang.org/pkg/compress/lzw/#NewReader
[3]https://golang.org/pkg/compress/zlib/#NewReader

However, even if they didn't (as bzip2 doesn't) it says nothing about
the implementation of something that does.

Dan

[Steven Penny]

>> - https://golang.org/pkg/compress/flate
>>
>> - https://golang.org/pkg/compress/lzw
>> - https://golang.org/pkg/compress/zlib
>
> All of these do.

whoops, yeah thats true. However my original point still stands. Three people
have replied now:

- Axel Wagner
- Bruno Albuquerque
- Dan Kortschak

and none has been able to provide a simple program that demonstrate a problem
that could arise from not closing gzip reader. So I am confident for now at
least that its not needed.

[Dan Kortschak]

On Mon, 2021-06-21 at 17:44 -0500, Steven Penny wrote:
> and none has been able to provide a simple program that demonstrate a
> problem that could arise from not closing gzip reader.

No, I gave a clear path that would lead to a case of a non-detected
error when Close is not called.

This seems like a really odd hill to die on, but it's your choice I
guess.

Dan

[Steven Penny]

> No, I gave a clear path that would lead to a case of a non-detected
> error when Close is not called.
>
> This seems like a really odd hill to die on, but it's your choice I
> guess.

You calling "go look through 3200 lines of Go code" (not including tests) [1] a
"clear path" is a dubious comment at best.

1. https://github.com/golang/go/tree/master/src/compress/flate

[Steven Hartland]

Internally close calls z.decompressor.Close() where decompressor is typically obtained via flat.NewReader(r) which states

NewReader returns a new ReadCloser that can be used to read the uncompressed version of r. If r does not also implement io.ByteReader, the decompressor may read more data than necessary from r. It is the caller's responsibility to call Close on the ReadCloser when finished reading.

The ReadCloser returned by NewReader also implements Resetter.

It's been some time now but I'm pretty sure I've seen gzip Reader fail silently when the Close error wasn't correctly checked, so there is a good reason why you should not only call Close but also confirm it returns nil. 

--
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...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAP8dQmsmqFqOu-iS9-e%3D3FZimG4nybdwM-d53JccaCuGoXJjRg%40mail.gmail.com.

[Dan Kortschak]

You asked a question. People here are genuinely trying to help you by
giving you their acquired knowledge of (collectively) over more than a
couple of decades of Go programming.

I spent 5 minutes looking at flate to find a path where an error can be
generated and not returned until Close is called; the part of the code
in particular that you want to look at is calls to compressor.step
which is assigned a method value (it should be straightforward to see
where this is assigned and what the assigned methods do).

However, the short answer is that the authors of the types included the
Close method for a reason; here your finding that bzip2.Reader doesn't
have a Close is a petard for you since it's clear that they're not just
added for the hell of it.

Dan

[Steven Penny]

> You asked a question. People here are genuinely trying to help you by
> giving you their acquired knowledge of (collectively) over more than a
> couple of decades of Go programming.

OK, so all all these decades of experience, why cant anyone produce a small
program to demonstrate the problem? Either they cant be bothered, or they dont
actually know how to create a program that would demonstrate a bad situation

that could arise from not closing gzip reader.

I am not questioning anyones knowledge, I am just asking for a demonstration,
rather than "do it because we said so".

[Bruno Albuquerque]

But it is not "because we said so". There were actual replies that pointed to specific issues that, in my opinion, were reason enough. To create a specific test case that would break (assuming there is not such a test in the standard library already. Worth checking) it would take some time for someone to dig what the specific error condition is and then write a sample program to show you such a case (it might be tricky without mocks as it might require generating errors from lower layers that are not easy to replicate in real life). This all takes time and considering you are the most interested on the output of this discussion, you could simply use the pointers that were given and check for yourself.

--
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...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAP8dQmvoR7COEsEB0PvuSCFC8cvF-9NuxTGBv7SZ06-phtekWw%40mail.gmail.com.

[Kurtis Rader]

On Mon, Jun 21, 2021 at 4:23 PM Steven Penny <srp...@gmail.com> wrote:

That is not a reasonable request. Everyone but you has been reasonable. They've also provided good reasons for the recommendation to call the Close method. That you have never observed a problem due to not calling the Close method does not mean a problem can't happen.

--

Kurtis Rader

Caretaker of the exceptional canines Junior and Hank

[Ian Lance Taylor]

On Mon, Jun 21, 2021 at 4:23 PM Steven Penny <srp...@gmail.com> wrote:
>

It's not clear to me what kind of demonstration you are looking for.
Your original example showed an interaction between os.Open and
os.Rename on Windows, and you showed closing the underlying file
without closing the gzip.Reader. I agree that in that sense nothing
bad will happen if you don't close the gzip.Reader. The only thing
that gzip.Reader.Close does is report whether there some error
occurred while reading the compressed data.

Would you be satisfied with a program that demonstrates that
gzip.Reader.Close can return a non-nil error?

Ian

[Steven Penny]

Thanks to all the replies, but looks like I got the answer from
another forum [1]. Copying here:

The Close() method was originally used to tear down the background
groutine used by the deflater. The background goroutine was removed
in in a June, 2011 change list [2]. The author of the CL wrote the
following in a comment on the CL [3]:

> Close used to tear down the goroutine associated with the reader, but now there isn't one. Probably Close will go away entirely, but that's a separate CL.

They never got around to removing the Close() method before the
compatibility guarantee [4] was introduced in the Go 1 release.

1. https://stackoverflow.com/a/68075823
2. https://github.com/golang/go/commit/07acc02a29ac74e7c0b
3. https://codereview.appspot.com/4548079#msg3
4. https://golang.org/doc/go1compat

[Dan Kortschak]

On Mon, 2021-06-21 at 18:22 -0500, Steven Penny wrote:
> I am not questioning anyones knowledge, I am just asking for a
> demonstration, rather than "do it because we said so".

https://play.golang.org/p/gwDnxVSQEM4

[Axel Wagner]

On Tue, Jun 22, 2021 at 1:23 AM Steven Penny <srp...@gmail.com> wrote:

OK, so all all these decades of experience, why cant anyone produce a small
program to demonstrate the problem?

I must say, I'm impressed by Dan taking the time to actually provide one.

My answer to this would have been: Because we would have to take time out of our lives just to prove a stranger on the internet, that he's wrong about something. And that's just not a convincing incentive.

You complain:

 

You calling "go look through 3200 lines of Go code" (not including tests) [1] a
"clear path" is a dubious comment at best.

And it's fair not wanting to wade through the code (I didn't). But we did give you a much clearer and simpler way: The API contract says you should call Close, so you should. This requires almost no time to confirm.

Again, the interest to write correct code should be yours here. It is unreasonable to complain that you would have to wade through the code and expect us to not only do that, but then also carefully craft a program which proves you wrong - when the only person who benefits from it is you and the only reason it's necessary is that you refuse to keep to a simple API contract.

Again, kudos to Dan. He certainly was a lot more willing to sacrifice his time for the benefit of a stranger than I would've been or is reasonable to expect.

Either they cant be bothered, or they dont
actually know how to create a program that would demonstrate a bad situation
that could arise from not closing gzip reader.

I am not questioning anyones knowledge, I am just asking for a demonstration,
rather than "do it because we said so".

--
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...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAP8dQmvoR7COEsEB0PvuSCFC8cvF-9NuxTGBv7SZ06-phtekWw%40mail.gmail.com.

[Brian Candler]

https://xkcd.com/386/

:-)

[Steven Penny]

On Mon, Jun 21, 2021 at 6:52 PM 'Dan Kortschak' wrote:
> https://play.golang.org/p/gwDnxVSQEM4

Thanks for the help, but I found a much simpler example:

https://play.golang.org/p/EcitH-85X6S

[Dan Kortschak]

On Tue, 2021-06-22 at 12:45 -0500, Steven Penny wrote:
> Thanks for the help, but I found a much simpler example:
>
> https://play.golang.org/p/EcitH-85X6S

Yes, trivial examples also exist.

[Steven Penny]

On Tue, Jun 22, 2021 at 4:31 PM 'Dan Kortschak' wrote:
> Yes, trivial examples also exist.

I thought one possible reason was `http#Response.Body` [1], because if
a response contains `Content-Encoding: gzip`, then Go will
automatically wrap the Body in a `gzip.Reader`. If that was the case,
then a Close method would be required, as Body being a
`io.ReadCloser`, it must have a `Read` and `Close` method. However
`gzip#Reader.Close` specifically says that it doesnt close the
underlying reader:

> Close closes the Reader. It does not close the underlying io.Reader.

So wrapping Body with gzip Reader directly would be bad, because the
Body would be left open on Close. The source code confirms that they
use a custom gzip type instead [2]. Interestingly, the Close method on
this type [3] closes the underlying reader, but doesnt bother to close
the gzip reader, even using `defer`. So I think it can be safe to omit
using gzip Reader Close in the general case.

1. https://golang.org/pkg/net/http#Response.Body
2. https://github.com/golang/go/blob/go1.16.5/src/net/http/transport.go#L2187
3. https://github.com/golang/go/blob/go1.16.5/src/net/http/transport.go#L2831-L2833

[a2800276]

 So I think it can be safe to omit
using gzip Reader Close in the general case.

In the general case it's also safe to ignore all errors, because by definition they only occur in exceptional conditions . 

More practically though, most programmers would probably prefer to follow guidance provided by the authors of the library they are using because by virtue of having written the library, the authors probably understand not only the library's implementation  and  technical constraints better than the user but  also tend to have a better grasp of the problem domain.

As a rule if I feel that the author of libraries I intend to use are totally inept morons and can't be trusted to not ask me to arbitrarily call random unnecessary functions I would shy away from using their library altogether.

[Jesper Louis Andersen]

On Wed, Jun 23, 2021 at 12:55 PM a2800276 <a280...@gmail.com> wrote:

More practically though, most programmers would probably prefer to follow guidance provided by the authors of the library they are using because by virtue of having written the library, the authors probably understand not only the library's implementation  and  technical constraints better than the user but  also tend to have a better grasp of the problem domain.

In addition: errors are markers for observability. That is, given errors, you can deduce how the internal state of the system went wrong, and what to do about it. As a rule: the more subtle the bug, the more aggressive your error reporting needs to be.

--

J.

[Steven Penny]

On Wed, Jun 23, 2021 at 5:55 AM a2800276 wrote:
> As a rule if I feel that the author of libraries I intend to use are totally inept morons and can't be trusted to not ask me to arbitrarily call random unnecessary functions I would shy away from using their library altogether.

I want to thank everyone who gave genuine replies to this topic.

However I think this thread has run its course. I got what I needed
from it, and at this point people just seem to be trolling/flaming
rather than being constructive. My last email was an honest referenced
reply after careful thought, so I dont appreciate the shitpost reply.

I am unsubscribing, thanks again.