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

Don't try to parse error responses with no body #4307

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

markusthoemmes
Copy link
Contributor

HEAD requests for instance return no body while still having all the relevant Content-Type headers set, causing unnecessary parsing errors. This skips further parsing for all requests that don't have any body to begin with.

Augments #4013.

HEAD requests for instance return no body while still having all the relevant Content-Type headers set, causing unnecessary parsing errors. This skips further parsing for all requests that don't have any body to begin with.

Signed-off-by: Markus Thömmes <markusthoemmes@me.com>
@markusthoemmes
Copy link
Contributor Author

If accepted, a backport to 2.8 would be neat 😅

Copy link
Member

@milosgajdos milosgajdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reluctantly approving as

  1. we explicitly warn in the README the client is deprecated which is the reason why it's been moved to the internal package
  2. we will be removing the client code in some future release TBD

If this code is somewhat important to you I'd strongly encourage you to consider replacing this client code with something that's maintained.

@markusthoemmes
Copy link
Contributor Author

markusthoemmes commented Mar 20, 2024

@milosgajdos I'm not well versed in this codebase rn, I was purely moving off of error messages I've seen in the deployed product when being run in mirror mode. Would that not use this code-path and am I on the wrong track there? 🤔


Edit: I've poked around and yeah, it seems like the client is in-use in the proxy code, so I guess from that PoV, it's still valid? Or are we looking at replacing it there as well?

@milosgajdos
Copy link
Member

Edit: I've poked around and yeah, it seems like the client is in-use in the proxy code, so I guess from that PoV, it's still valid? Or are we looking at replacing it there as well?

We've not discussed that at large -- but your observation is spot on; the only reason why the client code has not been removed completely is because it's used on the proxy.

Frankly, the proxy is in the state it is and would need a significant investment of effort to make it work better than it is now. But for the time being it'll remain in this codebase

@milosgajdos milosgajdos merged commit 94146f5 into distribution:main Mar 20, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants