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

[vstinner]

Merged, thanks for the fix.

I prefer to not backport the change to stable branches (3.13, 3.14), since it makes _pyrepl stricter and it may raise new exceptions. I prefer to wait to see how it goes in the main branch first. Maybe we need to catch some exceptions in some places.

[vstinner]

This change fixed the issue gh-132962 "windows console: -i parameter and redirected output gives error in 3.13".

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Windows Server 2022 NoGIL 3.x (tier-1) has failed when building commit 3abc03c.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1241/builds/7808) and take a look at the build logs.
4. Check if the failure is related to this commit (3abc03c) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1241/builds/7808

Failed tests:

• test_launcher

Failed subtests:

• test_search_major_2 - test.test_launcher.TestLauncher.test_search_major_2
• test_search_major_3 - test.test_launcher.TestLauncher.test_search_major_3

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "C:\Users\Administrator\buildarea\3.x.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\test\test_launcher.py", line 445, in test_search_major_3
    self.assertEqual("PythonCore", data["env.company"])
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 'PythonCore' != 'PythonTestSuite'
- PythonCore
+ PythonTestSuite


Traceback (most recent call last):
  File "C:\Users\Administrator\buildarea\3.x.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\test\test_launcher.py", line 465, in test_search_major_2
    self.assertEqual("PythonCore", data["env.company"])
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 'PythonCore' != 'PythonTestSuite'
- PythonCore
+ PythonTestSuite

[aisk]

I'm not familiar with the failed test, but I think it's not related to this change.

[vstinner]

Correct, the test_launcher failures are unrelated to this pyrepl change.