Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix EOF errors on http response with content and encoding headers #3438

Merged
merged 1 commit into from Nov 6, 2023

Conversation

mstoykov
Copy link
Collaborator

@mstoykov mstoykov commented Nov 2, 2023

Before this on a response which has a Content-Encoding value, but no body will error out with EOF as we still try to decode it.

As the code and the semantics of network requests mean that we might not know if we are going to be able to read something and whether we got and EOF because of network error or because there was no body - we need to figure out if we should try to read at all.

Unfortunately Content-Encoding: chunked exists which makes a more general solution much harder IMO.

As such the current solution is to short circuit on known status codes that do not have content. Namely all 1xx and 204 (aptly named no content) and 304 (not modified).

https://www.rfc-editor.org/rfc/rfc9110.html#section-6.4.1-8

Additionally a bare minimum tests was added reproducing what the user reported.

https://community.grafana.com/t/error-decompressing-response-body-eof-despite-http-request-returning-204/106927

Before this on a response which has a Content-Encoding value, but no
body will error out with EOF as we still try to decode it.

As the code and the semantics of network requests mean that we might
not know if we are going to be able to read something and whether we got
and EOF because of network error or because there was no body - we need
to figure out if we should try to read at all.

Unfortunately `Content-Encoding: chunked` exists which makes a more
general solution much harder IMO.

As such the current solution is to short circuit on known status codes
that do not have content. Namely all 1xx and 204 (aptly named no
content) and 304 (not modified).

https://www.rfc-editor.org/rfc/rfc9110.html#section-6.4.1-8

Additionally a bare minimum tests was added reproducing what the user
reported.

https://community.grafana.com/t/error-decompressing-response-body-eof-despite-http-request-returning-204/106927
@mstoykov mstoykov added the bug label Nov 2, 2023
@mstoykov mstoykov added this to the v0.48.0 milestone Nov 2, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (70c6284) 73.35% compared to head (166bf51) 73.34%.

❗ Current head 166bf51 differs from pull request most recent head d038178. Consider uploading reports for the commit d038178 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3438      +/-   ##
==========================================
- Coverage   73.35%   73.34%   -0.01%     
==========================================
  Files         259      259              
  Lines       19659    19662       +3     
==========================================
+ Hits        14420    14421       +1     
- Misses       4354     4355       +1     
- Partials      885      886       +1     
Flag Coverage Δ
ubuntu 73.28% <100.00%> (-0.01%) ⬇️
windows 73.19% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
lib/netext/httpext/compression.go 84.69% <100.00%> (+0.48%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mstoykov mstoykov merged commit 6dc5783 into master Nov 6, 2023
23 checks passed
@mstoykov mstoykov deleted the fixEOFOnResponseWithNoContent branch November 6, 2023 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants