bpo-36948: Fix NameError in urllib.request.URLopener.retrieve #13389
Conversation
isidentical
left a comment
There was a problem hiding this comment.
Do this PR needs a news entry?
@isidentical I have added one already at https://github.com/python/cpython/pull/13389/files#diff-1f45e92fbc7f2721e27165810e333552 |
|
I'm just asking, i dont think this is a change that requires a news entry. |
This is a user impacting bug where even though the function was deprecated it's now causing |
Lib/test/test_urllib.py
Outdated
| def test_urlopener_retrieve_file(self): | ||
| with self.assertWarns(DeprecationWarning): | ||
| try: | ||
| fd, tmp_file = tempfile.mkstemp() |
There was a problem hiding this comment.
Can't we use tempfile.NamedTemporaryFile and context management protocol to avoid handling resources manually?
There was a problem hiding this comment.
I guess I used it due to Windows permission error in previous CI runs where the created file was not readable but with mkstemp it's possible as seen from the docstring. It seems to be a windows only case but it seems to have other problem where the directory could be used by another process as seen in current CI run. Is there a reliable way to generate a temp file for this usecase?
The file is readable and writable only by the creating user ID.
If the operating system uses permission bits to indicate whether a
file is executable, the file is executable by no one. The file
descriptor is not inherited by children of this process.
There was a problem hiding this comment.
I think support.unlink() can be used to handle "it's being used by another process" errors under Windows.
Also, the order of self.addCleanup() calls is important. You might be bitten by a similar issue like d0f5bab.
There was a problem hiding this comment.
NamedTemporaryFile gives me that the file is not readable. Hence I used temp_dir and mkstemp to generate a more safer version of temp file for this test but I am sure I am not the only one having this problem in the testsuite.
There was a problem hiding this comment.
I don't think calling os.close() just after tempfile.mkstemp() makes sense. The temp directory will be cleaned up after the support.temp_dir() context manager and fd will be effectively invalid.
There was a problem hiding this comment.
Without calling os.close(fd) there will be an open file handle to the tempfile inside the directory and there will be a permission error that file handle is used by another process cannot be cleaned up as support.temp_dir context manager exits and due to unclosed fd. I guess Windows doesn't allow deleting directory when there is an open file handle to one of the files inside.
There was a problem hiding this comment.
Ah, now I understood what you are talking about, and, yes, your analysis is correct. I think the current version of the test is good and reliable enough. Thanks!
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
…directory. See if used by other process error is fixed with this in Windows.
|
I have resorted to using I have made the requested changes; please review again |
|
Thanks for making the requested changes! @berkerpeksag: please review the changes made to this pull request. |
|
Thanks @tirkarthi for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
|
GH-13422 is a backport of this pull request to the 3.7 branch. |
|
Thanks @berkerpeksag for the review. This is a 3.8 only issue so I guess the backport PR to 3.7 can be closed. |
Fix NameError in urllib.request.URLopener.retrieve by using correct imports.
https://bugs.python.org/issue36948