bpo-36948: Fix NameError in urllib.request.URLopener.retrieve by tirkarthi · Pull Request #13389 · python/cpython

tirkarthi · 2019-05-17T16:15:27Z

Fix NameError in urllib.request.URLopener.retrieve by using correct imports.

https://bugs.python.org/issue36948

isidentical

Do this PR needs a news entry?

tirkarthi · 2019-05-17T19:12:22Z

Do this PR needs a news entry?

@isidentical I have added one already at https://github.com/python/cpython/pull/13389/files#diff-1f45e92fbc7f2721e27165810e333552

isidentical · 2019-05-17T19:36:40Z

I'm just asking, i dont think this is a change that requires a news entry.

tirkarthi · 2019-05-17T20:16:15Z

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 NameError that used to work fine. IMO, it's a bug fix and requires a NEWS entry.

Lib/test/test_urllib.py

berkerpeksag · 2019-05-18T22:13:46Z

Lib/test/test_urllib.py

+    def test_urlopener_retrieve_file(self):
+        with self.assertWarns(DeprecationWarning):
+            try:
+                fd, tmp_file = tempfile.mkstemp()


Can't we use tempfile.NamedTemporaryFile and context management protocol to avoid handling resources manually?

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.

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.

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.

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.

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.

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!

Lib/test/test_urllib.py

bedevere-bot · 2019-05-18T22:19:38Z

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

…in Windows

…directory. See if used by other process error is fixed with this in Windows.

tirkarthi · 2019-05-19T03:14:50Z

I have resorted to using support.temp_dir to create a temporary directory for mkstemp . I guess the file being used error was to adding os.close(fd) to Cleanup where it's closed at the end of tests though the context manager support.temp_dir tries to clean it up on __exit__ causing error on cleaning up tmp_dir with an open file handle. So I have added os.close(fd) on the next line itself. I must admit that I resolved the issue with trial and error since I don't have access to Windows. Suggestions if any windows compatible test would be helpful too.

I have made the requested changes; please review again

bedevere-bot · 2019-05-19T03:14:53Z

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

Lib/test/test_urllib.py

miss-islington · 2019-05-19T13:40:09Z

Thanks @tirkarthi for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

…GH-13389) (cherry picked from commit c661b30) Co-authored-by: Xtreak <tir.karthi@gmail.com>

bedevere-bot · 2019-05-19T13:40:24Z

GH-13422 is a backport of this pull request to the 3.7 branch.

tirkarthi · 2019-05-19T13:42:59Z

Thanks @berkerpeksag for the review. This is a 3.8 only issue so I guess the backport PR to 3.7 can be closed.

tirkarthi added 2 commits May 17, 2019 21:40

Fix NameError in urllib.request.URLopener.retrieve

df42e6e

Add NEWS entry

9eafb47

the-knights-who-say-ni added the CLA signed label May 17, 2019

bedevere-bot added the awaiting review label May 17, 2019

Use localhost in local file test for Windows

63ef750

isidentical requested changes May 17, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels May 17, 2019

Refactor test for windows

ea3599c

isidentical approved these changes May 17, 2019

View reviewed changes

berkerpeksag requested changes May 18, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels May 18, 2019

tirkarthi added 4 commits May 19, 2019 07:23

Fix review comments

5e5d914

Add mkstemp for windows

2105943

Add combination of tmpdir and mkstemp to avoid file being used error …

15675e3

…in Windows

Close file descriptor since context manager needs to remove the temp …

6061de0

…directory. See if used by other process error is fixed with this in Windows.

bedevere-bot added awaiting change review and removed awaiting changes labels May 19, 2019

berkerpeksag reviewed May 19, 2019

View reviewed changes

Lib/test/test_urllib.py Outdated Show resolved Hide resolved

Mixin before TestCase

7defa55

berkerpeksag approved these changes May 19, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 19, 2019

berkerpeksag added the needs backport to 3.7 label May 19, 2019

berkerpeksag merged commit c661b30 into python:master May 19, 2019

bedevere-bot removed the awaiting merge label May 19, 2019

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2019

bpo-36948: Fix NameError in urllib.request.URLopener.retrieve (python…

bec59f1

…GH-13389) (cherry picked from commit c661b30) Co-authored-by: Xtreak <tir.karthi@gmail.com>

bedevere-bot removed the needs backport to 3.7 label May 19, 2019

Uh oh!

Conversation

tirkarthi commented May 17, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isidentical left a comment

Choose a reason for hiding this comment

Uh oh!

tirkarthi commented May 17, 2019

Uh oh!

isidentical commented May 17, 2019

Uh oh!

tirkarthi commented May 17, 2019

Uh oh!

Uh oh!

berkerpeksag May 18, 2019

Choose a reason for hiding this comment

Uh oh!

tirkarthi May 19, 2019

Choose a reason for hiding this comment

Uh oh!

berkerpeksag May 19, 2019

Choose a reason for hiding this comment

Uh oh!

tirkarthi May 19, 2019

Choose a reason for hiding this comment

Uh oh!

berkerpeksag May 19, 2019

Choose a reason for hiding this comment

Uh oh!

tirkarthi May 19, 2019

Choose a reason for hiding this comment

Uh oh!

berkerpeksag May 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented May 18, 2019

Uh oh!

tirkarthi commented May 19, 2019

Uh oh!

bedevere-bot commented May 19, 2019

Uh oh!

Uh oh!

miss-islington commented May 19, 2019

Uh oh!

bedevere-bot commented May 19, 2019

Uh oh!

tirkarthi commented May 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tirkarthi commented May 17, 2019 •

edited by bedevere-bot

Loading