Skip to content

bpo-45919: Use WinAPI GetFileType() in is_valid_fd() #30082

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
merged 3 commits into from
Dec 13, 2021

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Dec 13, 2021

@corona10
Copy link
Member Author

corona10 commented Dec 13, 2021

During startup, is_valid_fd() in Python/pylifecycle.c is called to validate stdin, stdout, and stderr. Performance isn't critical here, but every bit helps in reducing startup time. In my tests, implementing this check in Windows via GetFileType((HANDLE)_get_osfhandle(fd)) is 5-6 times faster than close(dup(fd)). For example:

I got the same performance enhancement by measuring QueryPerformanceCounter.

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. I just proposed a minor coding style change (it's up to you).

@eryksun
Copy link
Contributor

eryksun commented Dec 13, 2021

The _Py_BEGIN_SUPPRESS_IPH and _Py_END_SUPPRESS_IPH macros are used to temporarily disable the MSVC invalid parameter handler for the _get_osfhandle() call. The use of these macros can be removed from the cases that are limited to POSIX systems. For example:

    int res;
    _Py_BEGIN_SUPPRESS_IPH
    res = fcntl(fd, F_GETFD);
    _Py_END_SUPPRESS_IPH
    return res >= 0;

becomes simply:

    return fcntl(fd, F_GETFD) >= 0;

@eryksun
Copy link
Contributor

eryksun commented Dec 13, 2021

The comment that discusses the function should be updated to remove Windows from the platforms that use dup().

@eryksun
Copy link
Contributor

eryksun commented Dec 13, 2021

Was it verified that fcntl(fd, F_GETFD) addresses the case that forced macOS to use fstat() instead of dup()? If not, create_stdio() could fail with an EBADF error in macOS.

@vstinner
Copy link
Member

vstinner commented Dec 13, 2021

Was it verified that fcntl(fd, F_GETFD) addresses the case that forced macOS to use fstat() instead of dup()? If not, create_stdio() could fail with an EBADF error in macOS.

See the comment:

   [bpo-30225](https://bugs.python.org/issue30225): On macOS Tiger, when stdout is redirected to a pipe and the other
   side of the pipe is closed, dup(1) succeed, whereas fstat(1, &st) fails with
   EBADF. FreeBSD has similar issue ([bpo-32849](https://bugs.python.org/issue32849)).

In case of doubt, I prefer to continue using fcntl() which always works. On BSD systems, dup() seems to implement less checks.

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.

@corona10 corona10 merged commit 191c431 into python:main Dec 13, 2021
@corona10 corona10 deleted the bpo-45919 branch December 13, 2021 12:58
@eryksun
Copy link
Contributor

eryksun commented Dec 13, 2021

In case of doubt, I prefer to continue using fcntl() which always works. On BSD systems, dup() seems to implement less checks.

@vstinner, the original change was to use fstat() instead of dup(). bpo-30225 makes no reference to fcntl(). @tiran added the fcntl() call in bpo-45915, with __APPLE__ in the allowed platforms, but there was no discussion that it actually checks the pipe state in macOS to handle the case of bpo-30225.

@eryksun
Copy link
Contributor

eryksun commented Dec 13, 2021

The comment that discusses the function should be updated to remove Windows from the platforms that use dup().

The out of date comment was not updated in the merge.

@vstinner
Copy link
Member

The out of date comment was not updated in the merge.

@corona10 wrote #30090

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants