-
-
Notifications
You must be signed in to change notification settings - Fork 33.3k
gh-140691: Close FTP connection on error in urllib.request #140694
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
Lib/test/test_urllib2.py
Outdated
| mock_fw = mock.MagicMock() | ||
| mock_fw.retrfile.side_effect = ftplib.error_temp("Connection timed out") | ||
|
|
||
| with mock.patch.object(urllib.request.FTPHandler, 'connect_ftp', |
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.
We want to test what happens when fw.retrfile fails, so need to mock urllib.request.ftpwrapper.
The mock here is mocking out a lot more than necessary, should only need to mock the class urllib.request.ftpwrapper method retrfile to raise an exception
Lib/test/test_urllib2.py
Outdated
|
|
||
| handler = urllib.request.FTPHandler() | ||
|
|
||
| with warnings.catch_warnings(record=True) as warning_list: |
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.
This is part of the testing needed, will also need to use test.support to check that there are no "unraisable exceptions" (which is what this one ends up being when it isn't correctly cleaned up). Look at other tests for examples
Lib/test/test_urllib2.py
Outdated
| close_called = [] | ||
| original_close = urllib.request.ftpwrapper.close | ||
|
|
||
| def tracked_close(self): |
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.
The mock library tools that can use to check counts called: https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_called_once
Lib/urllib/request.py
Outdated
| if fw is not None: | ||
| fw.close() |
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.
This will close fw, but leave the closed connection in CacheFTPHandler's cache.
|
Thanks for tackling this! I left some notes in the issue. I'll let you take it from here but let me know if I should look into this more. |
d44e990 to
f5394c2
Compare
When FTP request fails after the control connection has been established (e.g., timeout during file retrieval), the connection
was not being closed, causing a ResourceWarning.
This fix ensures that the FTP connection is properly closed in the exception handler of ftp_open().
The new test uses mocking to simulate FTP timeout errors and verifies that:
close()method is calledResourceWarningis raised