suggestion: response is always None at this point, and passing it is the risky part of the plumbing.
It's reset to None at the top of each loop iteration and only assigned on the success path, so the except branch can only ever pass response=None — _should_retry then falls through to response = e.response anyway. That makes the parameter dead.
It's also a duck-typing trap: Retry.increment and retries.sleep expect a urllib3.response.HTTPResponse and call get_redirect_location(), .status, and (on urllib3 1.26, permitted by the new floor) .getheader("Retry-After"). A requests.Response has none of those. Today the truthiness guards short-circuit because all three members of _RETRY_REQUEST_EXCEPTIONS are raised without a response attached — but any future change that surfaces a retryable RequestException carrying one turns a would-be retry into AttributeError, escaping the except MaxRetryError handler as an unrelated error type.
Passing response=None explicitly (the retry decision is already driven entirely by exception type) removes both the dead parameter and the trap.
Originally posted by @rtibblesbot in #348 (comment)
suggestion:
responseis alwaysNoneat this point, and passing it is the risky part of the plumbing.It's reset to
Noneat the top of each loop iteration and only assigned on the success path, so theexceptbranch can only ever passresponse=None—_should_retrythen falls through toresponse = e.responseanyway. That makes the parameter dead.It's also a duck-typing trap:
Retry.incrementandretries.sleepexpect aurllib3.response.HTTPResponseand callget_redirect_location(),.status, and (on urllib3 1.26, permitted by the new floor).getheader("Retry-After"). Arequests.Responsehas none of those. Today the truthiness guards short-circuit because all three members of_RETRY_REQUEST_EXCEPTIONSare raised without aresponseattached — but any future change that surfaces a retryableRequestExceptioncarrying one turns a would-be retry intoAttributeError, escaping theexcept MaxRetryErrorhandler as an unrelated error type.Passing
response=Noneexplicitly (the retry decision is already driven entirely by exception type) removes both the dead parameter and the trap.Originally posted by @rtibblesbot in #348 (comment)