-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Raise exception for shutdown on a connection already released to the pool #3601
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
Conversation
@sethmlarson Could you please review this? Thank you! |
src/urllib3/response.py
Outdated
@@ -1073,6 +1076,10 @@ def readable(self) -> bool: | |||
return True | |||
|
|||
def shutdown(self) -> None: | |||
if self._released_conn: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if self._released_conn: | |
if not self._connection: |
There's no need for an extra attribute here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@D-stefaang Thank you for the review. I have removed the extra attribute now.
|
||
|
||
class TestShutdownOnConnectionReleasedToPool(HypercornDummyServerTestCase): | ||
def test_shutdown_on_connection_released_to_pool(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please move this test to be after test_cleanup_on_connection_error
? We don't want to many HypercornDummyServerTestCase
as they start a new server each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pquentin Thank you for your review. I have moved the new test to be after test_cleanup_on_connection_error
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM. Sorry for the 2 months delay. Please ask for $100 on Open Collective, with links to the issue and pull request and post back the link here.
@pquentin Thank you for the opportunity to contribute! |
Closes: #3581