Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34k
gh-132888: Fix Windows API error checking in pyrepl.windows_console#144248
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
base:main
Are you sure you want to change the base?
Conversation
| WaitForSingleObject.argtypes= [HANDLE,DWORD] | ||
| WaitForSingleObject.restype=DWORD | ||
| OutHandle=GetStdHandle(STD_OUTPUT_HANDLE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
GetStdHandlecan returnINVALID_HANDLE_VALUE and should be checked, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is at the top level of the file, so it will be executed when importing this module. Therefore, we can't raise an exception here. What we can do is check if the result isINVALID_HANDLE_VALUE and set it toNone or zero.
We will pass theOutHandle to other Windows APIs, likeReadConsoleInput. There, we will check the result and raise an exception usingget_last_error().
However, if we don't check the result and just keep it asINVALID_HANDLE_VALUE, the whole process won't change. KeepingINVALID_HANDLE_VALUE might actually be a better choice, since 0 may be valid in some situations. That's why MS introducedINVALID_HANDLE_VALUE instead of using zero.
Therefore, I think it's better to just keep the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree to all your points 👍
chris-eibl commentedJan 26, 2026 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
In general this lgtm. I've built locally and did a few very basic manual tests like redirecting stdout / stderr. anymore (#132962 (comment)). But turning the silent swallowing into raising exceptions might bite us (although most probably the right thing to do), like you've seen in the failing tests in headless mode. Most probably the swallowing just let another error surface slightly later, anyway, so I think it is ok to fail early and loud? |
aisk commentedJan 27, 2026
Hi, thank you for your review. I'm quite unsure whether we should throw the error early during initialization, but since this is a private module and its main usage is in an environment with a console, and the error is just raised when there is no console, I think it's acceptable to raise the error early |
Uh oh!
There was an error while loading.Please reload this page.
ctypes.GetLastError()in windows_console.py #132888