fix: natively check for namedpip existance on windows - #384
Conversation
|
Please sign the commit with |
leongdl
left a comment
There was a problem hiding this comment.
Please re-submit with commit --amend -s, and also fix the linting error:
class BlenderClient(ClientInterface):
def __init__(self, server_path: str) -> None:
super().__init__(server_path=server_path)
print(f"BlenderClient: Blender Version {bpy.app.version_string}")
Oh no! 💥 💔 💥
1 file would be reformatted, 58 files would be left unchanged.
hatch run fmt all should do it.
|
|
||
| # Call Win32 WaitNamedPipeW with a 0ms timeout | ||
| # Returns non-zero if a pipe instance exists | ||
| result = ctypes.windll.kernel32.WaitNamedPipeW(pipe_path, 0) |
There was a problem hiding this comment.
WaitNamedPipeW with nTimeOut = 0 is not a 0 ms timeout. 0 is NMPWAIT_USE_DEFAULT_WAIT, which tells Windows to use the default time-out value that the server passed to CreateNamedPipe. So this call can block for however long the adaptor server configured (potentially seconds) rather than returning immediately.
Related: the error codes accepted below do not cover the timeout case. When the pipe exists but every instance is currently connected, WaitNamedPipeW fails with ERROR_SEM_TIMEOUT (121), not 231/5 — so an existing-but-busy pipe would be reported as missing and main() would raise OSError even though the adaptor is up. ERROR_PIPE_BUSY (231) is what CreateFile returns, not WaitNamedPipe.
Suggest passing a small explicit timeout (e.g. 1) and treating ERROR_SEM_TIMEOUT as “exists”, or dropping WaitNamedPipeW entirely and just attempting the CreateFileW open the client is about to do anyway.
|
|
||
| # GetLastError check | ||
| # ERROR_PIPE_BUSY (231) or ERROR_ACCESS_DENIED (5) means the pipe exists! | ||
| last_error = ctypes.GetLastError() |
There was a problem hiding this comment.
ctypes.GetLastError() is not a reliable way to read the error from the preceding call here. The windll cache shares function objects process-wide and does not save the thread's last-error value, so any intervening ctypes/CRT activity between the WaitNamedPipeW call and this line can clobber it. The documented pattern is to load the DLL with use_last_error=True and read via ctypes.get_last_error():
kernel32 = ctypes.WinDLL("kernel32", use_last_error=True)
kernel32.WaitNamedPipeW.argtypes = (ctypes.c_wchar_p, ctypes.c_uint32)
kernel32.WaitNamedPipeW.restype = ctypes.c_int32
...
last_error = ctypes.get_last_error()Declaring argtypes/restype also matters: without them the DWORD timeout and the BOOL return are passed/interpreted as plain C int, which is fragile across the Win64 calling convention.
6540e7d to
532e0f1
Compare
Done - let me know if the other 🤖 remarks should be addressed 👍 |
732212f to
6d5cfce
Compare
Why os.path.exists() Always Fails on Python 3.13 for Windows Pipes - CPython 3.13 Refactoring: In Python 3.13, CPython optimized os.stat() / os.path.exists() on Windows to use the GetFileAttributesExW Win32 API call. - NPFS Incompatibility: The Windows Named Pipe File System (NPFS / \\.\pipe\) does not support file attribute queries. When GetFileAttributesExW is called on a named pipe, Windows returns ERROR_FILE_NOT_FOUND (WinError 2). - The Result: Because GetFileAttributesExW fails, Python 3.13's os.path.exists(r"\\.\pipe\...") always evaluates to False, regardless of how long you wait or whether the pipe is active and healthy. The previous check worked in Python 3.10/3.11 because older CPython versions fell back to CreateFileW inside os.stat(). Signed-off-by: Patrick Jahns <github@patrickjahns.de>
6d5cfce to
f8cc3e0
Compare
leongdl
left a comment
There was a problem hiding this comment.
Thank you for the contribution.
Blender 5.2 switched to Python313 and with that switch the BlenderClient always exited with an Error
What was the problem/requirement? (What/Why)
See #383 for complete background. In summary - CPython changed the behaviour for
os.stat.exists()between 311 and 313.background
What was the solution? (How)
What is the impact of this change?
How was this change tested?
Please run the integration tests and paste the results below
If
installer/was modified or a file was added/removed fromsrc/, then update the installer tests and post the test results belowWas this change documented?
Is this a breaking change?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.