pgzip
pgzip copied to clipboard
klauspost
gunzip: improve EOF handling
This fixes #38 and #39, though I'm not entirely sure if you're happy with this approach.
To solve #39, we switch from using a channel for the block pool and instead use a sync.Pool. This does have the downside that the read-ahead goroutine can now end up allocating more blocks than the user requested. If this is not acceptable I can try to figure out a different solution for this problem. By using sync.Pool, there is no issue of blocking on a goroutine channel send when there are no other threads reading from it.
To solve #38, some extra io.EOF special casing was needed in both WriteTo and Read. I think that these changes are reasonable -- it seems as though z.err should never store io.EOF (and there were only a few cases where it would -- which I've now fixed), but let me know what you think.
Fixes #38 Fixes #39
Signed-off-by: Aleksa Sarai [email protected]
@cyphar Thank you for looking through my ancient code. I remember it being quite painful when I looked through it a year ago.
This does have the downside that the read-ahead goroutine can now end up allocating more blocks than the user requested
@cyphar It this without limit? There must be a limit to how far it can read ahead, otherwise you will easily run the system out of memory.
@cyphar It this without limit? There must be a limit to how far it can read ahead, otherwise you will easily run the system out of memory.
Yeah it's without limit -- the issue is that sync.Pool doesn't have a way of limiting the size of the pool -- so it is possible you'd end up with an endlessly growing number of blocks. I will have to come up with another way of fixing the deadlock issue -- I think the issue with the channel approach taken is that the channel is getting full for some reason. I'll take another look.
@cyphar It would be great if you could look at that. Unbounded decompression is an issue. Thanks!
Got bitten by this, hard. @klauspost unbounded decompression is an issue, but currently the results are just wrong, which may lead to data loss.
Could we consider at least getting it to return data? I'd rather see my server blow up than lose data.
I'll take another look at how to fix the deadlock issue without switching to sync.Pool...
Details escape me, but I would think that persisting errors, so future calls are rejected should be enough.
Problem is that if you do an io.Copy() on a reader that has buffered fully, you won't get an error. It'll just, silently, copy 0 bytes and call it done.
I'd be happy with an error, even if it was just "TODO". But not silently eating data, that's dangerous.
@klauspost The io.EOF handling is fixed in a separate commit (gunzip: handle io.EOF errors correctly in WriteTo), but the unit tests require the deadlocks to be fixed.
@rubenv
Problem is that if you do an io.Copy() on a reader that has buffered fully, you won't get an error. It'll just, silently, copy 0 bytes and call it done.
io.Copy()'s semantics are that it returns nil when the end of the file is reached, so you won't get io.EOF if there's no data to read. I guess the issue is that the compressor hasn't generated all the data? Is this issue also fixed by this PR? Do you have a test case I can add?
The sync.Pool change doesn't work for us here and will cause serious regressions. As you proposed initially, please consider a solution that retains the original pool.
You are welcome to submit separate PRs, if that will simplify code review.