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

compress/gzip: Enforce that reader implements io.ByteReader with multistream disabled. #53491

Closed
Shaptic opened this issue Jun 21, 2022 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@Shaptic
Copy link

Shaptic commented Jun 21, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18.1 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

n/a

What did you do?

If you pass an *os.File (which implements io.Reader) to gzip.Reader and disable multi-stream (.Multistream(false)), the .Reset(r) call will return EOF after the first gzip stream despite there being more streams in the file.

https://go.dev/play/p/NEdhYQNfKds

Notice that we wrote two chunks to the file (named "hello" and "world") but only one was read (the first, "hello").

What did you expect to see?

One of two things:

  • a read of every gzip stream in the file
  • an error indicating that the reader does not implement io.ByteReader

What did you see instead?

Only the first stream is read.

This behavior is very subtly noted in the documentation:

The underlying reader must implement io.ByteReader in order to be left positioned just after the gzip stream.

However, a runtime check and error for interface compatibility would be far more intuitive.

@ianlancetaylor
Copy link
Contributor

CC @dsnet

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 22, 2022
@dsnet
Copy link
Member

dsnet commented Jun 23, 2022

I believe this is working as intended.

Regarding documentation, I'm not sure I agree that this is "subtle". The documented requirement for io.ByteWriter appears with Reader.Multistream which must be called to engage in multistream mode. Had it only been somewhere (e.g., on NewWriter), I can imagine that being subtle.

Regarding a change in semantics, we can't do that. One of the valid use-cases for Reader.Multistream is to read each individual GZIP file while holding onto the same Reader. In this situation, the internal buffering of Reader doesn't matter since we're not throwing it away and reconstructing it anew.

@dsnet
Copy link
Member

dsnet commented Jul 14, 2022

Closing as there's nothing actionable to do.

@dsnet dsnet closed this as completed Jul 14, 2022
@golang golang locked and limited conversation to collaborators Jul 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants