Skip to content
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

Refactor nt._path_is* & nt._path_[l]exists #118507

Closed
nineteendo opened this issue May 2, 2024 · 8 comments
Closed

Refactor nt._path_is* & nt._path_[l]exists #118507

nineteendo opened this issue May 2, 2024 · 8 comments
Labels
type-feature A feature request or enhancement

Comments

@nineteendo
Copy link
Contributor

nineteendo commented May 2, 2024

Feature or enhancement

Proposal:

Quoting @eryksun:

I agree that all of these _path_is* and _path_[l]exists helpers are a lot of code to maintain, taken together. It could be refactored into smaller inline helper functions that can be reused, which would also make the code more readable.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

Linked PRs

@nineteendo nineteendo added the type-feature A feature request or enhancement label May 2, 2024
@nineteendo
Copy link
Contributor Author

Eryk, do you have any ideas to clean them up (besides the simpler error handling)?

@eryksun

This comment was marked as resolved.

@nineteendo
Copy link
Contributor Author

Good start, we probably need a follow_symlinks option for _testFileTypeByName to use LSTAT() though.

@eryksun

This comment was marked as resolved.

@eryksun

This comment was marked as resolved.

@nineteendo
Copy link
Contributor Author

Did you speed something up, because it's only 3 lines less than the old code.

@eryksun
Copy link
Contributor

eryksun commented May 8, 2024

My goal was to remove duplicated code and divide the work into two separate operations that can be understood and modified independently, written in a way that I think is easy to understand. This reduces the maintenance burden. I also added internal support for checking for mount points, in case we implement _path_isjunction().

I added a GetFileType() check in the by-handle code. When combined with the diskOnly parameter, the GetFileType() check makes it safer to check an open file descriptor since the check is implemented directly in the I/O manager without trying to acquire the file lock. If GetFileInformationByHandleEx() is called on a handle for a synchronous-mode pipe, it could block indefinitely. For example, with the current implementation in 3.12:

>>> pr, pw = os.pipe()
>>> threading.Thread(target=os.read, args=(pr, 1)).start()
>>> os.path.isfile(pr)
^C

Also, given the system's named-pipe filesystem (NPFS) support for basic file information (anonymous pipes are also in NPFS, but they're like unlinked open files in POSIX), the GetFileType() check avoids classifying pipes as regular files. Again, with the current implementation in 3.12:

>>> pr, pw = os.pipe()
>>> os.path.isfile(pr)
True

@nineteendo
Copy link
Contributor Author

OK, that explains why the code isn't much shorter. I added a test for the second issue. The first one seems difficult to test.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 21, 2024
…r other cases (pythonGH-118755)

(cherry picked from commit b641825)

Co-authored-by: Nice Zombies <nineteendo19d0@gmail.com>
zooba pushed a commit that referenced this issue May 22, 2024
…r cases (GH-118755)

(cherry picked from commit b641825)

Co-authored-by: Nice Zombies <nineteendo19d0@gmail.com>
nineteendo added a commit to nineteendo/cpython that referenced this issue May 22, 2024
zooba pushed a commit that referenced this issue May 22, 2024
This refactoring will make future backports easier without changing behaviours,
apart from correcting a bug when passing a pipe to `ntpath.isfile`.
zooba added a commit to zooba/cpython that referenced this issue Jul 4, 2024
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants