Skip to content

gh-101881: os: support blocking functions on Windows #101882

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 2 commits into from
Feb 16, 2023

Conversation

RayyanAnsari
Copy link
Contributor

@RayyanAnsari RayyanAnsari commented Feb 13, 2023

Use the GetNamedPipeHandleState and SetNamedPipeHandleState Win32 API functions to add support for os.get_blocking and os.set_blocking.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ghost
Copy link

ghost commented Feb 13, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@RayyanAnsari
Copy link
Contributor Author

RayyanAnsari commented Feb 13, 2023

Tested with this script:

import os
import sys

r, w = os.pipe()

os.set_blocking(r, sys.argv[1] == 'True')
os.set_blocking(w, sys.argv[1] == 'True')

if (os.get_blocking(r) and os.get_blocking(w)):
    print("blocking")
else:
    print("non-blocking")

try:
    while True:
        os.write(w, b'hello')
except BlockingIOError:
    print("blocked")

Output:

PS C:\Users\Rayyan\cpython> PCbuild\amd64\python_d.exe ..\test_blocking.py False
non-blocking
blocked
PS C:\Users\Rayyan\cpython> PCbuild\amd64\python_d.exe ..\test_blocking.py True
blocking
[script hangs]

@RayyanAnsari RayyanAnsari force-pushed the windows-os-blocking branch 5 times, most recently from b34c8a4 to 658e4f2 Compare February 14, 2023 17:08
@RayyanAnsari RayyanAnsari force-pushed the windows-os-blocking branch 5 times, most recently from 0277c03 to f853e83 Compare February 14, 2023 22:40
@RayyanAnsari RayyanAnsari requested a review from eryksun February 14, 2023 22:40
@RayyanAnsari RayyanAnsari force-pushed the windows-os-blocking branch 3 times, most recently from 939920f to ba175a1 Compare February 15, 2023 18:02
@RayyanAnsari
Copy link
Contributor Author

Should now be ready to merge.

@eryksun
Copy link
Contributor

eryksun commented Feb 15, 2023

Refer to [MS-FSCC]: FilePipeLocalInformation for details about how the write-quota available depends on whether it's a handle for the server end or client end. Note that it documents the underlying NT API, so the names are different.

WriteQuotaAvailable (4 bytes): A 32-bit unsigned integer that contains the write quota, in bytes, for the named pipe. If the NamedPipeEnd field is set to FILE_PIPE_CLIENT_END, the WriteQuotaAvailable field is the remaining InboundQuota field available. If the NamedPipeEnd field is set to FILE_PIPE_SERVER_END, the WriteQuotaAvailable field is the remaining OutboundQuota field available.

It would be nice if the Windows API exposed WriteQuotaAvailable, which is precisely the value that we need. Limiting a write to exactly what's available would allow more non-blocking writes to succeed.

@eryksun eryksun requested a review from a team February 15, 2023 19:35
@bedevere-bot
Copy link

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.

Handle erroring operations on non-blocking pipes by reading the _doserrno code.
Limit writes on non-blocking pipes that are too large.
Use the GetNamedPipeHandleState and SetNamedPipeHandleState Win32 API functions to add support for os.get_blocking and os.set_blocking.
@RayyanAnsari
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from zooba February 16, 2023 11:37
@zooba
Copy link
Member

zooba commented Feb 16, 2023

Thanks for the contribution! Just FYI, there's no need to force push updates to PRs for our project - we always squash merge and edit the commit messages ourselves. All that a force push will do is reset the review status, which makes it hard to see the most recent changes.

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

Successfully merging this pull request may close these issues.

5 participants