gh-132888: Fix Windows API error checking in pyrepl.windows_console

[aisk]

• Issue: Wrong use of ctypes.GetLastError() in windows_console.py #132888

[chris-eibl]

GetStdHandle can return INVALID_HANDLE_VALUE and should be checked, too?

[aisk]

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 is INVALID_HANDLE_VALUE and set it to None or zero.

We will pass the OutHandle to other Windows APIs, like ReadConsoleInput. There, we will check the result and raise an exception using get_last_error().

However, if we don't check the result and just keep it as INVALID_HANDLE_VALUE, the whole process won't change. Keeping INVALID_HANDLE_VALUE might actually be a better choice, since 0 may be valid in some situations. That's why MS introduced INVALID_HANDLE_VALUE instead of using zero.

Therefore, I think it's better to just keep the current implementation.

[chris-eibl]

I agree to all your points 👍

[chris-eibl]

In general this lgtm. I've built locally and did a few very basic manual tests like redirecting stdout / stderr.
The changes where raise WinError(GetLastError()) are replaced with raise WinError(get_last_error()) are definitely ok, because we've also raised before and now won't just show the misleading

OSError: [WinError 0] The operation completed successfully.

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]

In general this lgtm. I've built locally and did a few very basic manual tests like redirecting stdout / stderr. The changes where raise WinError(GetLastError()) are replaced with raise WinError(get_last_error()) are definitely ok, because we've also raised before and now won't just show the misleading

OSError: [WinError 0] The operation completed successfully.

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?

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