Skip to content

Short-circuit raise_for_status code for statuses that will not raise …#5792

Closed
ptmcg wants to merge 1 commit intopsf:masterfrom
ptmcg:raise_for_status_short_circuit
Closed

Short-circuit raise_for_status code for statuses that will not raise …#5792
ptmcg wants to merge 1 commit intopsf:masterfrom
ptmcg:raise_for_status_short_circuit

Conversation

@ptmcg
Copy link
Copy Markdown

@ptmcg ptmcg commented Apr 11, 2021

…an exception

This short-circuit code will skip the steps for decoding response.reason if the status code is not an exception-raising status code.

@sigmavirus24
Copy link
Copy Markdown
Contributor

Decoding the reason certainly isn't that expensive in reality. Why add this complexity here? What does this gain anybody?

@ptmcg
Copy link
Copy Markdown
Author

ptmcg commented Apr 12, 2021

Early exit from an exception-raising routine, when it can be quickly determined that no exception will be raised, would be considered by some to be less complexity, not more. At run-time it saves several additional if-tests for all successful statuses, and for those reading the code, it clearly shows that only status codes >= 400 are really of any interest when raising an exception.

(I could understand if there were some possible restructuring of HTTP status codes in the future, but this status code organization is pretty cast in stone now.)

@nateprewitt
Copy link
Copy Markdown
Member

Hi @ptmcg, thanks for the initiative here! Unfortunately, I think I'm in agreement with @sigmavirus24. This is an optimization that adds extra code clutter without providing a tangible user benefit.

I did some quick benchmarking and performance difference is measured in fractions of a microsecond. There's never going to be a situation where we accept latency involved with sending a request across a network but are concerned about sub-microsecond precision in post processing.

@nateprewitt nateprewitt closed this Sep 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants