querier: exhaust response body before closing#4429
Conversation
|
Reading entire body may take a while or simply never finish, if malicious client keeps sending data (one byte per second :)). It might be good idea to impose some time or size limit on this reading. |
Aren't requests time-bound via deadlines set on contexts? If not, could you please suggest something specific that I could use here? On the Thanos side, we've been using such a construct for a very long time without any problems: https://github.com/thanos-io/thanos/blob/main/pkg/runutil/runutil.go#L45-L49 |
There may or may not be timeout. Still, I would suggest to read say max 1 KB of remaining body – that should cover most cases. If there is more data in the body, just close it, and drop the connection: _ = io.Copy(ioutil.Discard, io.LimitReader(response.Body, 1024))
_ = response.Body.Close() |
Exhaust the whole body of the response before closing to facilitate re-use of keep-alive connections as per the documentation here: https://pkg.go.dev/net/http#Client.Get ``` Caller should close resp.Body when done reading from it. ``` Also some discussion here: https://groups.google.com/g/golang-nuts/c/148riho42sU cortexproject#4428 Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
GiedriusS
force-pushed
the
feature/exhaust_body
branch
from
2787ddd to
514f93d
Compare
|
@pstibrany thanks for the suggestion, I have applied it and tested that with this connections are still properly re-used. |
|
Note that Cortex uses |
Exhaust the whole body of the response before closing to facilitate re-use of keep-alive connections as per the documentation here: https://pkg.go.dev/net/http#Client.Get ``` Caller should close resp.Body when done reading from it. ``` Also some discussion here: https://groups.google.com/g/golang-nuts/c/148riho42sU cortexproject#4428 Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> Signed-off-by: Alvin Lin <alvinlin@amazon.com>
3 participants
What this PR does:
Exhaust the whole body of the response before closing to facilitate
re-use of keep-alive connections as per the documentation here:
https://pkg.go.dev/net/http#Client.Get
Also some discussion here:
https://groups.google.com/g/golang-nuts/c/148riho42sU
#4428
Which issue(s) this PR fixes:
It fixes a part of #4428.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]Not sure if any CHANGELOG changes are needed 🤔