Adding sync.Pool to Decompress middleware #1699

pafuent · 2020-11-27T03:06:58Z

Fixing a http.Request.Body leak on the decompress middleware that were not properly Close
Removing the defer on the call to gzip.Reader, because that reader is already exhausted after the call to io.Copy

Fixing a http.Request.Body leak on the decompress middleware that were not properly Close Removing the defer on the call to gzip.Reader, because that reader is already exausted after the call to io.Copy

codecov · 2020-11-27T03:07:54Z

Codecov Report

Merging #1699 (2386e17) into master (502cce2) will decrease coverage by 0.01%.
The diff coverage is 86.20%.

@@            Coverage Diff             @@
##           master    #1699      +/-   ##
==========================================
- Coverage   84.88%   84.86%   -0.02%     
==========================================
  Files          29       29              
  Lines        1945     1969      +24     
==========================================
+ Hits         1651     1671      +20     
- Misses        187      189       +2     
- Partials      107      109       +2

Impacted Files	Coverage Δ
middleware/decompress.go	`88.37% <85.71%> (-6.37%)`	⬇️
middleware/compress.go	`84.31% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 502cce2...2386e17. Read the comment docs.

pafuent · 2020-11-27T03:12:01Z

@arun0009 Could you please review this PR?
Sadly I didn't had the time to review the PR in which you added the Decompress middleware, and I saw it when it was merged.
Besides the use of a sync.Pool (which I bring from the Compress middleware), I think that the gzip.Reader could be Close without the defer and that the http.request.Body that is replaced at the end, should be closed. When the Body is not changed, is the Server the one responsible for closing it.

pafuent · 2020-11-27T03:21:13Z

Codecov is failing because I'm no testing some error code branches, that sadly I don't know how to test. If someone have any suggestion, I'll be glad to implement it.

Here are the benchmarks comparison for this change. In this case I did 10 iterations instead of the 5 performed on our GitHub action

name          old time/op    new time/op    delta
Decompress-2    2.21µs ± 6%    2.08µs ± 6%   -6.01%  (p=0.003 n=10+10)

name          old alloc/op   new alloc/op   delta
Decompress-2    5.49kB ± 0%    4.91kB ± 0%  -10.60%  (p=0.000 n=10+9)

name          old allocs/op  new allocs/op  delta
Decompress-2      12.0 ± 0%      11.0 ± 0%   -8.33%  (p=0.000 n=10+10)

arun0009 · 2020-11-27T05:48:20Z

middleware/decompress.go

+				gr.Close()
+				pool.Put(gr)
+
+				b.Close() // http.Request.Body is closed by the Server, but because we are replacing it, it must be closed here


good catch!

arun0009 · 2020-11-27T08:08:28Z

Codecov is failing because I'm no testing some error code branches, that sadly I don't know how to test. If someone have any suggestion, I'll be glad to implement it.

Here are the benchmarks comparison for this change. In this case I did 10 iterations instead of the 5 performed on our GitHub action
name          old time/op    new time/op    delta
Decompress-2    2.21µs ± 6%    2.08µs ± 6%   -6.01%  (p=0.003 n=10+10)

name          old alloc/op   new alloc/op   delta
Decompress-2    5.49kB ± 0%    4.91kB ± 0%  -10.60%  (p=0.000 n=10+9)

name          old allocs/op  new allocs/op  delta
Decompress-2      12.0 ± 0%      11.0 ± 0%   -8.33%  (p=0.000 n=10+10)

One way of testing InternalServerError or pool error handling is by using interfaces. Here's an example that would cover 91.9% of code base https://gist.github.com/arun0009/f8a19659cf956b82e3abadc2211e3be0

pafuent · 2020-11-28T02:20:44Z

Thanks for the advice @arun0009

arun0009 · 2020-12-01T08:25:58Z

middleware/decompress.go

 				var buf bytes.Buffer
 				io.Copy(&buf, gr)
+
+				gr.Close()
+				pool.Put(gr)


@pafuent - you are putting gr back to pool twice? On line 100 and 110? Also, whats resetting gr before putting it back in pool? of is it the usage of gr.Reset making sure that it's reset before reading new request body?

Line 100 because the call to Reset returned an error, so the middleware is skipped (for empty bodies) or returns the error (all the rest of the errors). And the line 110 is the happy path 😉
I'm not using a defer just to return it to the Pool as fast as is possible to avoid an allocation if a new request find the Pool empty.
Reset() requires the io.Reader, so I need to perform that after getting one instance from the Pool

lammel · 2020-12-07T00:33:02Z

Looks like you also added the missing test for the error path.
Are we ready to merge @pafuent ?

pafuent · 2020-12-11T03:55:58Z

@lammel, yes it's ready to be merged

Adding sync.Pool to Decompress middleware

14e020b

Fixing a http.Request.Body leak on the decompress middleware that were not properly Close Removing the defer on the call to gzip.Reader, because that reader is already exausted after the call to io.Copy

pafuent self-assigned this Nov 27, 2020

arun0009 reviewed Nov 27, 2020

View reviewed changes

Increasing Decompress Middleware coverage

2386e17

arun0009 reviewed Dec 1, 2020

View reviewed changes

lammel merged commit 194129d into labstack:master Dec 11, 2020

pafuent deleted the improve_decompress_middleware branch December 14, 2020 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding sync.Pool to Decompress middleware #1699

Adding sync.Pool to Decompress middleware #1699

pafuent commented Nov 27, 2020

codecov bot commented Nov 27, 2020 •

edited

Loading

pafuent commented Nov 27, 2020

pafuent commented Nov 27, 2020

arun0009 Nov 27, 2020

arun0009 commented Nov 27, 2020

pafuent commented Nov 28, 2020

arun0009 Dec 1, 2020 •

edited

Loading

pafuent Dec 3, 2020

lammel commented Dec 7, 2020

pafuent commented Dec 11, 2020

Adding sync.Pool to Decompress middleware #1699

Adding sync.Pool to Decompress middleware #1699

Conversation

pafuent commented Nov 27, 2020

codecov bot commented Nov 27, 2020 • edited Loading

Codecov Report

pafuent commented Nov 27, 2020

pafuent commented Nov 27, 2020

arun0009 Nov 27, 2020

Choose a reason for hiding this comment

arun0009 commented Nov 27, 2020

pafuent commented Nov 28, 2020

arun0009 Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

pafuent Dec 3, 2020

Choose a reason for hiding this comment

lammel commented Dec 7, 2020

pafuent commented Dec 11, 2020

codecov bot commented Nov 27, 2020 •

edited

Loading

arun0009 Dec 1, 2020 •

edited

Loading