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

@eryksun

Feature or enhancement

Proposal:

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:

gh-117841: Add C implementation of ntpath.lexists #117842

Linked PRs

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

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

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

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

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.

gh-118507 : Refactor `nt._path_is*` to improve applicability for othe…

pythongh-118507 : Refactor `nt._path_is*` to improve applicability fo…

gh-118507 : Refactor `nt._path_is*` to improve applicability for othe…

pythongh-118507 : Refactor `nt._path_is*` to improve applicability fo…

gh-118507 : Refactor ntpath native functions (gh-119381)

gh-118507: Amend news entry to mention ntpath.isfile bugfix (GH-120817)

pythongh-118507: Amend news entry to mention ntpath.isfile bugfix

gh-118507: Amend news entry to mention ntpath.isfile bugfix (GH-120817)

pythongh-118507: Amend news entry to mention ntpath.isfile bugfix (py…

pythongh-118507 : Refactor `nt._path_is*` to improve applicability fo…

pythongh-118507: Amend news entry to mention ntpath.isfile bugfix (py…

Navigation Menu

Repository navigation

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

Feature or enhancement

Proposal:

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Linked PRs

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

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

Description

Feature or enhancement

Proposal:

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Linked PRs

Activity

nineteendo commented on May 6, 2024

eryksun commented on May 7, 2024

nineteendo commented on May 7, 2024

eryksun commented on May 7, 2024

eryksun commented on May 7, 2024

nineteendo commented on May 8, 2024

eryksun commented on May 8, 2024

nineteendo commented on May 8, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

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