Add support for Brotli and Zstandard compression #1082
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #1082 +/- ##
==========================================
+ Coverage 72.79% 72.87% +0.08%
==========================================
Files 133 133
Lines 9890 9929 +39
==========================================
+ Hits 7199 7236 +37
- Misses 2276 2277 +1
- Partials 415 416 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1082 +/- ##
==========================================
+ Coverage 72.79% 72.91% +0.12%
==========================================
Files 133 133
Lines 9890 9949 +59
==========================================
+ Hits 7199 7254 +55
- Misses 2276 2278 +2
- Partials 415 417 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1082 +/- ##
==========================================
+ Coverage 72.79% 72.87% +0.08%
==========================================
Files 133 133
Lines 9890 9929 +39
==========================================
+ Hits 7199 7236 +37
- Misses 2276 2277 +1
- Partials 415 416 +1
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #1082 +/- ##
==========================================
+ Coverage 72.79% 72.87% +0.08%
==========================================
Files 133 133
Lines 9890 9929 +39
==========================================
+ Hits 7199 7236 +37
- Misses 2276 2277 +1
- Partials 415 416 +1
Continue to review full report at Codecov.
|
lib/netext/httpext/request.go
Outdated
| case CompressionTypeBr: | ||
| brotlireader := brotli.NewReader(resp.Body) | ||
| // brotli.Reader doesn't implement io.ReadCloser, so wrap it | ||
| resp.Body = ioutil.NopCloser(brotlireader) |
There was a problem hiding this comment.
Unfortunately, we need to close the original resp.Body, so we can reuse the network connection. From the golang http docs:
If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close. If the Body is not both read to EOF and closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.
And now, when I was reviewing this code, I noticed that the old k6 code above for gzip and zlib also has the same error :man_facepalming... zlib.NewReader() and gzip.NewReader() return io.ReadCloser implementations, but they don't actually close the underlying. For gzip:
Close closes the Reader. It does not close the underlying io.Reader.
And for zlib:
Calling Close does not close the wrapped io.Reader originally passed to NewReader.
There was a problem hiding this comment.
All of the cases (the new ones and the original) could probably be fixed with a simple wrapper struct around an io.Reader (the compression readers) and an io.ReadCloser (the original body). Its Close() method can optionally close the compression reader (by type asserting if it's actually io.ReadCloser) and then always close the original body (so we can reuse connections). Basically, instead of NopCloser, we should have a OneOrBothCloser 😄
There was a problem hiding this comment.
It is also very important that we actually read the whole body which might not happen if there are errors (for example in the decoding).
So I propose that a defer with Reading the whole body and closing it after that is added, and for all decoders to get a NopClosers so that they don't close the body prematurely
imiric
force-pushed
the
feat/http-zstd-brotli-compression
branch
from
f295044
to
78154b8
Compare
imiric
force-pushed
the
feat/http-zstd-brotli-compression
branch
from
78154b8
to
3249d6d
Compare
|
BTW, I'm a big fan of Conventional Commits, which I used here, but let me know if you prefer plain messages as currently used in the repo. |
There was a problem hiding this comment.
I can't comment on the previous body closing discussion, since it's "outdated" in the github interface, so I'll do it here. Beside the issue I noted inline about the closure parameters, there's another another very minor one with the current code - if there's no compression of the response, we'll be closing the body twice - on lines 217 (after the closure is fixed) and 256. Though I'm not sure if that is actually a real issue, since maybe the second one will be a no-op?
But what I think @mstoykov meant was that we can just handle the body closing with defers only where it's required, i.e. keep the defer on line 215 (after fixing it), remove the closing on line 256 and add defers for closing only the compression decoders that need closing? To me your current solution seems more readable, and since out of the decompressors only zstd implements io.WriterTo (a faster path io.Copy takes when its source argument supports it), I don't think we need to change it.
Regarding the CLA, we fixed the typo you found in it and now apparently you have to accept it again, sorry 😄
And thanks for mentioning conventional commits, I've added an issue about investigating and potentially adopting them (and alternative commit styles): #1086
lib/netext/httpext/request.go
Outdated
| rc := &readCloser{resp.Body} | ||
| // Ensure that the entire response body is read and closed, e.g. in case of decoding errors | ||
| defer func() { | ||
| _, _ = io.Copy(ioutil.Discard, resp.Body) |
There was a problem hiding this comment.
Unfortunately this won't work - by the time this deferred function is executed, resp.Body might have been changed by the code below. You need to capture the current value and pass it as an argument to the closure.
imiric
force-pushed
the
feat/http-zstd-brotli-compression
branch
from
3249d6d
to
1ea3827
Compare
There was a problem hiding this comment.
LGTM.
I prefer if we don't tie our hands with wrappers which will make the fast path of io.Copy not possible. But we will need some kind of wrapper either way as in my proposed scenario the decompressors would've been getting the wrapped Body ...
While testing I found out we haven't tested the support for unsupported compression algorithms ( ;) ). While it doesn't throw an error and you can decompress in JS (which is what we wanted:)) it also doesn't enter this switch statement as it was expected instead it just sets err and then that isn't checked. Which means that it won't set any kind of error that can be checked in JS.
The whole thing needs even more work because when we get out of the readResponse if there is an error we will currently panic instead of returning the error and the response ... so .. I don't know. Open an issue @na-- ?
This was fairly straightforward to add, thanks to the existing code structure.
It unfortunately adds a lot of 3rd party code because of the new dependencies, but I agree with your current convention of keeping that in the repo, since it makes it very visible.
Cheers,
Ivan
Closes: #776, #1080