Repository navigation
gh-140691: Close FTP connection on error in urllib.request - #140694
codenamenam wants to merge 0 commit into
Conversation
Sorry, something went wrong.
| 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.
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
Sorry, something went wrong.
|
|
||
| handler = urllib.request.FTPHandler() | ||
|
|
||
| with warnings.catch_warnings(record=True) as warning_list: |
There was a problem hiding this comment.
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
Sorry, something went wrong.
| close_called = [] | ||
| original_close = urllib.request.ftpwrapper.close | ||
|
|
||
| def tracked_close(self): |
There was a problem hiding this comment.
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
Sorry, something went wrong.
| if fw is not None: | ||
| fw.close() |
There was a problem hiding this comment.
This will close fw, but leave the closed connection in CacheFTPHandler's cache.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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: