gunzip: improve EOF handling #40
Conversation
This matches NewWriter, and reduces the possibility of forgetting to update one of the initialisation functions when making changes. Signed-off-by: Aleksa Sarai <[email protected]>
This stops us from causing goroutine deadlocks when we re-add a buffer after the stream has been read and the read-ahead goroutine is dead. Note that with this change, it is possible for more blocks to be allocated than the user requested. Signed-off-by: Aleksa Sarai <[email protected]>
WriteTo should not return io.EOF because it is assumed to have io.Copy semantics (namely, on success you return no error -- even if there were no bytes copied). Several parts of WriteTo would return io.EOF -- all of which need to be switched to special-case io.EOF. In addition, Read would save io.EOF in z.err in some specific corner cases -- these appear to be oversights and thus are fixed to not store io.EOF in z.err. Signed-off-by: Aleksa Sarai <[email protected]>
Before this patchset, these tests would either lock up or fail with spurrious EOF errors. 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. |
@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 |
|
@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 |
|
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 I'd be happy with an error, even if it was just "TODO". But not silently eating data, that's dangerous. |
|
@klauspost The
|
|
The You are welcome to submit separate PRs, if that will simplify code review. |
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 usingsync.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.EOFspecial casing was needed in bothWriteToandRead. I think that these changes are reasonable -- it seems as thoughz.errshould never storeio.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]