Skip to content

[WIP] A workaround for Python 3.6 WindowsConsoleIO breaking with FDCapture #2462

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

segevfiner
Copy link
Contributor

@segevfiner segevfiner commented Jun 2, 2017

Python 3.6 implemented unicode console handling for Windows. This works by reading/writing to the raw console handle using {Read,Write}ConsoleW.

The problem is that we are going to dup2 over the stdio file descriptors when doing FDCapture and this will CloseHandle the handles used by Python to write to the console. Though there is still some weirdness and the console handle seems to only be closed randomly and not on the first call to CloseHandle, or maybe it gets reopened with the same handle value when we suspend capturing.

The workaround in this case will reopen stdio with a different fd which also means a different handle by replicating the logic in "Py_lifecycle.c:initstdio/create_stdio".

Obviously not very pretty, I don't know what other side effects this might have and I'm not even sure this is the right way to go about this. Since the std handles are getting clobbered, other code which does GetStdHandle, or something similar, is also affected. Meaning that this is only a partial fix.

For example: colorama breaks just as randomly. But it ignores the errors and all colorama features just randomly stop working (e.g. Colors stop working). Adding error handling to colorama.winterm reveals the very same exception. Essentially colorama caches the std handles in colorama.win32.handles and it breaks, even without _WindowsConsoleIO being used by Python, due to FDCapture.

Of course, who knows what other stuff might get broken by this. And I'm still baffled by why it's random and only have a guess as to why (Described above).

Also note that I currently ignored the case if sys.stdout != sys.__stdout__ and similarly for the other std.std*. I'm not sure what is the right thing to do in such a case...

I'm using the Pytest test suite and this to reproduce the issue (Running with pytest -v):

import time
import random
import pytest


@pytest.mark.parametrize("i", range(1000))
def test_random_sleep(i, capfd):
    time.sleep(random.random() * 0.2)

See also

PR Checklist

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Add a new news fragment into the changelog folder
    • name it $issue_id.$type for example (588.bug)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of removal, feature, bugfix, vendor, doc or trivial
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
  • Target: for bugfix, vendor, doc or trivial fixes, target master; for removals or features target features;
  • Make sure to include reasonable tests for your change if necessary

Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:

  • Add yourself to AUTHORS;

Python 3.6 implemented unicode console handling for Windows. This works
by reading/writing to the raw console handle using
``{Read,Write}ConsoleW``.

The problem is that we are going to ``dup2`` over the stdio file
descriptors when doing ``FDCapture`` and this will ``CloseHandle`` the
handles used by Python to write to the console. Though there is still some
weirdness and the console handle seems to only be closed randomly and not
on the first call to ``CloseHandle``, or maybe it gets reopened with the
same handle value when we suspend capturing.

The workaround in this case will reopen stdio with a different fd which
also means a different handle by replicating the logic in
"Py_lifecycle.c:initstdio/create_stdio".

See pytest-dev/py#103
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, please add a news fragment so we can merge it

@The-Compiler
Copy link
Member

The-Compiler commented Jun 2, 2017

@RonnyPfannschmidt Not so fast - it is marked as WIP, breaks the checks, and would be nice if someone could test this on Windows 😉

@segevfiner
Copy link
Contributor Author

@RonnyPfannschmidt OK if you like it as is. Though lets make sure it at least passed CI first. (I had a wrong if condition). I will push an update soon.

Do note that any code doing GetStdHandle or similar is still affected. The handle returned by GetStdHandle seems to change after this happens, and like the issue itself, it happens randomly. Like I already mentioned: colorama. Which I will open a separate issue for. So we might need a SetStdHandle or similar to workaround more cases of this. Note that colorama is imported before this runs though. I can also think of pyreadline being affected.

@RonnyPfannschmidt
Copy link
Member

hmm, oh, i was indeed a bit quick - but i like the basic setup, lets sort out the technical issues

@segevfiner
Copy link
Contributor Author

segevfiner commented Jun 2, 2017

@RonnyPfannschmidt @The-Compiler I do think it would be nice if we can first figure this out completely before merging. Like I mentioned GetStdHandle, and who knows what else...

It probably also changes the fd numbers of the sys.std* objects, required obviously to avoid dup2 closing them later. But still...

EDIT: Note that appveyor is likely not using a Windows console to run the tests. (Something like winpty). So this code probably doesn't run there.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 92.133% when pulling 01ed6df on segevfiner:py36-windowsconsoleio-workaround into f826b23 on pytest-dev:master.

@nicoddemus
Copy link
Member

Agree, I would like to take this for a spin on my computer before merging this.

@segevfiner
Copy link
Contributor Author

segevfiner commented Jun 2, 2017

See http://bugs.python.org/issue30555. If this is solved in CPython's side this hack will not be necessary anymore. Since it will likely be a while until it's solved, if it's even decided to solve this in CPython's side. We can still implement a workaround and remove it/gate it on an upper CPython version, when and if, CPython implements it's own fix.

@nicoddemus
Copy link
Member

@segevfiner first of all thanks for all the work you've put into this. I've read briefly your bug reports both here, in py and in CPython, and also I'm following closely your PR to fix this problem in CPython itself. Great work! 🙇

Hopefully your python/cpython#1927 will get accepted as it affects other projects.

Other than that I've verified that this fixes the problem for me on my Windows system, so I'm 👍 to add this workaround to pytest: it only gets into effect for Python 3.6 users on Windows and is cleanly written so as to separate it from the normal code.

@segevfiner anything else you want to do with it, or can I merge it as it is?

@segevfiner
Copy link
Contributor Author

@nicoddemus It should be okay to merge. We just need to remember to disable it conditionally or remove it when and if this is fixed in CPython. 😉

@nicoddemus nicoddemus merged commit 1863b7c into pytest-dev:master Jun 3, 2017
@nicoddemus
Copy link
Member

Awesome, thanks again! 👍

@segevfiner segevfiner deleted the py36-windowsconsoleio-workaround branch June 4, 2017 04:27
prabhuramachandran added a commit to pypr/compyle that referenced this pull request Feb 28, 2019
On Windows with Python >= 3.6.0, the console handling seems to have
changed and this seems to break our capturing the file descriptor to
suppress the compilation messages on the console.  pytest has a fix
here: pytest-dev/pytest#2462 but we essentially
do not do any capturing on windows for these Python versions as this fix
doesn't seem to help us here.  Hopefully this will be resolved in
Python itself at some point but for now this error was breaking any
compilation using compyle on Windows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants