Skip to content

bpo-39916: Use os.scandir() as context manager in Path.glob(). #18880

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

Merged

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Mar 9, 2020

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Would it also be worthwhile to add a brief test case in test_pathlib to make sure a ResourceWarning is no longer raised when Path.glob() is interrupted? For example:

def test_interrupt_path_glob(self):
    rc, out, err = assert_python_ok('-c', """if True:
         import os
         from signal import SIGINT
         from pathlib import Path
         from test.support import TESTFN

         BASE = os.path.realpath(TESTFN)
         test_path = Path(BASE)
         for _ in test_path.glob("fileA"):
             # Simulate interruption in first iteration
             pid = os.getpid()
             os.kill(pid, SIGINT)
        """)
    # Ensure ResourceWarning didn't occur
    self.assertNotIn("ResourceWarning", err.decode())

The above will likely have to be adjusted, but I wanted to check if something along those lines was worth pursuing before investing more time into it.

@serhiy-storchaka
Copy link
Member Author

Thank you for your test @aeros, but it does not work. It is passed with unpatched pathlib. This is because at the time when when you get an item from the glob iterator you already have finished iterating the scandir iterator and closed it. I have not add tests for this PR because It is very uneasy. I would need to monkey-patch scandir and create a detailed working implementation of scandir, DirEntry, etc which fails at specific moment. And keep it synchronized with future changes in the main implementation. It is too much work for tiny benefit.

@aeros
Copy link
Contributor

aeros commented Mar 10, 2020

@serhiy-storchaka Ah, I see. The above was mostly a very rough draft idea, I was unaware of what the actual implementation of the test would involve (but figured you might have an idea). Thanks for the explanation.

With that in mind, I would agree that it's not worthwhile to go through that much effort (and added long-term maintenance cost) for a single minor test.

@serhiy-storchaka serhiy-storchaka merged commit 704e206 into python:master Mar 11, 2020
@miss-islington
Copy link
Contributor

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

@serhiy-storchaka serhiy-storchaka deleted the path-glob-scandir-cm branch March 11, 2020 16:42
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 11, 2020
@bedevere-bot
Copy link

GH-18934 is a backport of this pull request to the 3.8 branch.

@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 11, 2020
miss-islington added a commit that referenced this pull request Mar 11, 2020
)

(cherry picked from commit 704e206)

Co-authored-by: Serhiy Storchaka <[email protected]>
miss-islington added a commit that referenced this pull request Mar 11, 2020
)

(cherry picked from commit 704e206)

Co-authored-by: Serhiy Storchaka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants