GH-100573: Fix server hang caused by os.stat() on named pipe (Windows)

[gvanrossum]

• Issue: Calling os.stat() on a named pipe used by asyncio.ProactorEventLoop.start_serving_pipe() will raise OSError #100573

[gvanrossum]

@kumaraditya303 Any chance you have time to review this? And maybe @eryksun too?

The only possibly controversial thing is that I added self.call_soon(loop_accept_pipe) to the OSError except clause as well, so that other OS errors don't do the same thing and disable the server.

[eryksun]

In case the implementation of os.stat() changes, what basically needs to be tested is whether a pipe connection that's closed immediately causes the server to die, such as os.close(os.open(ADDRESS, os.O_RDONLY)).

[kumaraditya303]

I tried it and it does not reproduces the error. Note that I am using Windows 11.

[gvanrossum]

Yeah, I found the same problem. I also had to add two await asyncio.sleep(0) calls after server.close() to make the final close(open()) call fail as expected.

So I think I'd rather merge this as is, using os.stat().

[eryksun]

All that os.stat() is doing that's even remotely relevant here is calling h = CreateFileW(ADDRESS, ...), doing a bit of work that takes a little time, and then calling CloseHandle(h).

os.stat() won't be relevant to the error at all if Microsoft provides a better way to implement os.stat() that doesn't require opening a handle to the pipe, or if we decide that we should just inspect a "\\.\PIPE\" path as a string in os.stat() instead of opening a handle.

[gvanrossum]

Then how would you fix the test? os.close(os.open()) doesn’t fail without the fix.

[eryksun]

Maybe os.open() actually isn't fast enough to trigger the race condition reliably. Try _winapi.CloseHandle(_overlapped.ConnectPipe(ADDRESS)).

[eryksun]

I forgot that os.stat() also calls GetFileAttributesW() on the pipe while the handle is open. This tries to open a second pipe connection before the first one has been closed. The following reproduces the scenario:

h=_overlapped.ConnectPipe(ADDRESS) try: _winapi.CloseHandle(_overlapped.ConnectPipe(ADDRESS)) exceptOSErrorase: ife.winerror!=_overlapped.ERROR_PIPE_BUSY: raisefinally: _winapi.CloseHandle(h)

In the test, 2/5 attempts at opening a second pipe connection fail with ERROR_PIPE_BUSY, so this error is intentionally ignored.

[gvanrossum]

Maybe we don’t need a test. All this is irrelevant for the fix. I can either delete the test, keep the PR as is, or close it unresolved. (Someone else can try again.)

[eryksun]

The code in my last comment reliably triggers the problem for me because it ensures that the client connects and disconnects from the server's next pipe instance, leaving the pipe in a broken state before the server calls ConnectNamedPipe().

If it comes to closing the PR or deleting the test, I vote for deleting the test. It should have been designed from the beginning to handle the broken pipe error. Not every aspect of a correct design has to be tested.

[eryksun]

Similarly here, os.close(os.open(ADDRESS, os.O_RDONLY)) will raise FileNotFoundError after the server has closed.

[kumaraditya303]

For that we need to wait for closing the server so asyncio.sleep(0) is required.

[gvanrossum]

Doesn't look like that's needed with the latest probe code.

[kumaraditya303]

LGTM

[gvanrossum]

Please re-review, I've added Eryk's code. I've confirmed that it reproduces the bug and refactored the test so the probe code only needs to be written once.

[miss-islington]

Thanks @gvanrossum for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

[bedevere-bot]

GH-101019 is a backport of this pull request to the 3.11 branch.

[bedevere-bot]

GH-101020 is a backport of this pull request to the 3.10 branch.