Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-104530: Use native Win32 condition variables#104531

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
zooba merged 6 commits intopython:mainfromadr26:condvar
Feb 2, 2024

Conversation

@adr26
Copy link
Contributor

@adr26adr26 commentedMay 16, 2023•
edited by bedevere-bot
Loading

Update Python to using native Win32 condition variable support, by setting_PY_EMULATED_WIN_CV to0.

A small fix toPyCOND_TIMEDWAIT() was required to correctly report condition variable timeouts.

After doing this, I found that_listdir_windows_no_opendir() was crashing becausePy_END_ALLOW_THREADS is overwriting Win32's GLE and the error codeERROR_NO_MORE_FILES fromFindNextFileW() was being wiped out:

       Py_BEGIN_ALLOW_THREADS       result=FindNextFileW(hFindFile,&wFileData);       Py_END_ALLOW_THREADS       /* FindNextFile sets error to ERROR_NO_MORE_FILES if           it got to the end of the directory. */       if (!result&&GetLastError()!=ERROR_NO_MORE_FILES) {

I debugged this (using iDNA/TTD) and found thatSleepConditionVariableSRW() was clearing the Win32 GLE inPy_END_ALLOW_THREADS:

0:008> tTime Travel Position: 6D3A9:38ntdll!RtlSetLastWin32Error+0x46:00007ffb`26d43316 894868          mov     dword ptr [rax+68h],ecx ds:0000000f`a9a8c068=000000120:008> !gleLastErrorValue: (Win32) 0x12 (18) - There are no more files.LastStatusValue: (NTSTATUS) 0 - STATUS_SUCCESS0:008> tBreakpoint 0 hitTime Travel Position: 6D3A9:39ntdll!RtlSetLastWin32Error+0x49:00007ffb`26d43319 8b442450        mov     eax,dword ptr [rsp+50h] ss:0000000f`ab52d400=000000000:008> !gleLastErrorValue: (Win32) 0 (0) - The operation completed successfully.LastStatusValue: (NTSTATUS) 0 - STATUS_SUCCESS0:008> kn7# Child-SP          RetAddr               Call Site00 0000000f`ab52d3b0 00007ffb`24766f9d     ntdll!RtlSetLastWin32Error+0x4901 0000000f`ab52d400 00007ffb`247b2b47     KERNELBASE!BaseSetLastNTError+0x1d02 0000000f`ab52d430 00007ffa`722ff5d3     KERNELBASE!SleepConditionVariableSRW+0x3703 (Inline Function) --------`--------     python312!PyCOND_TIMEDWAIT+0x23 [C:\src\cpython\Python\condvar.h @ 285] 04 0000000f`ab52d470 00007ffa`72142665     python312!take_gil+0xb3 [C:\src\cpython\Python\ceval_gil.c @ 381] 05 (Inline Function) --------`--------     python312!PyEval_RestoreThread+0x11 [C:\src\cpython\Python\ceval_gil.c @ 711] 06 0000000f`ab52d4c0 00007ffa`7214883e     python312!_listdir_windows_no_opendir+0x315 [C:\src\cpython\Modules\posixmodule.c @ 4165]

I have fixed this by adding code toPyEval_RestoreThread to save and restore the Win32 GLE, and removing the code inPyThread_tss_get() andPyThread_get_key_value() that does the same (as it's no longer required).

cc:@ericsnowcurrently

@ericsnowcurrently
Copy link
Member

I'll defer to@zooba and other Windows experts on this one.

@zooba
Copy link
Member

I definitely want the fixes in this PR, even if we don't change the default setting for CVs.

My main concern is destabilising things -@encukou has just spent days tracking down a Windows deadlock, and I'd hate to give him a whole lot of new ones ;)

But let's see what the buildbots say

@zoobazooba added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelFeb 1, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@zooba for commitfb37931 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelFeb 1, 2024
@encukou
Copy link
Member

My main concern is destabilising things -@encukou has just spent days tracking down a Windows deadlock, and I'd hate to give him a whole lot of new ones ;)

Eh, I think there are more deadlocks waiting to be solved even without this PR.
If this is how things should be on Windows, now is the time to merge it.

@zooba
Copy link
Member

Alright, let's YOLO it 😆

Easy enough to change the setting to disable later on if it turns out to be trouble, but alphas are definitely the time to turn it on.

@zoobazooba merged commitb3f0b69 intopython:mainFeb 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@zoobazoobaAwaiting requested review from zooba

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@adr26@ericsnowcurrently@zooba@bedevere-bot@encukou@serhiy-storchaka
[8]ページ先頭
©2009-2025 Movatter.jp