Skip to content
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

Stdout hijacking in api.py is not thread safe #6125

Closed
randomstuff opened this issue Jan 2, 2019 · 7 comments · Fixed by #6750
Closed

Stdout hijacking in api.py is not thread safe #6125

randomstuff opened this issue Jan 2, 2019 · 7 comments · Fixed by #6750
Labels

Comments

@randomstuff
Copy link

See tomv564/pyls-mypy#19, AFAIU mypy api.py relying on sys.stdout overriding is not thread-safe and causes issues when sys.stout and sys.stderr are used concurrently.

@gvanrossum
Copy link
Member

This is a good catch. It may be a bit tricky to fix, but we’re open to PRs. There probably aren’t too many places where stdout or street are used, but passing something in may require a bunch of messy signature changes.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 2, 2019

It would be better to explicitly pass the stdout and stderr objects instead of modifying them in sys. Would you like to contribute a fix? This should be a pretty easy one.

Here's a summary of what's involved for anybody interested in helping with this:

  • Don't modify sys.stdout and sys.stderr in mypy.api.
  • Instead, pass relevant StringIO objects to mypy.main.main as arguments. These can default to sys.stdin and sys.stderr in main.
  • Use these arguments when we generate error messages.
    • No need to update dmypy, stubgen and logging to use these, since they aren't relevant when using api.py.

elkhadiy added a commit to elkhadiy/mypy that referenced this issue Jan 2, 2019
Overriding sys.stdout and sys.stderr in mypy.api is not threadsafe.
This causes problems sometimes when using the api in pyls for example.
@elkhadiy
Copy link

elkhadiy commented Jan 2, 2019

Erm... Well it seems Github happily shows any reference of an issue from any fork whatsoever... that's good to know.

I wanted to wait for tomorrow so my colleague could test it on his setup since he was more "lucky" than me in getting the race conditions to "work" on his machine(tm). I guess nothing's wrong in putting the PR up for review now.

@emmatyping
Copy link
Collaborator

@elkhadiy, take the time you need to make a good change set. Don't worry we won't jump on you :)

elkhadiy added a commit to elkhadiy/mypy that referenced this issue Jan 2, 2019
Overriding sys.stdout and sys.stderr in mypy.api is not threadsafe.
This causes problems sometimes when using the api in pyls for example.
elkhadiy added a commit to elkhadiy/mypy that referenced this issue Jan 2, 2019
Overriding sys.stdout and sys.stderr in mypy.api is not threadsafe.
This causes problems sometimes when using the api in pyls for example.
elkhadiy added a commit to elkhadiy/mypy that referenced this issue Jan 2, 2019
Overriding sys.stdout and sys.stderr in mypy.api is not threadsafe.
This causes problems sometimes when using the api in pyls for example.
@AnOctopus
Copy link
Contributor

What remains to be done about this issue? It causes a lot of problems on my setup (using pyls-mypy and spacemacs) and I'd be willing to do some work to fix it but the changes @elkhadiy made on their fork seem to already cover the needed changes.

@emmatyping
Copy link
Collaborator

@AnOctopus I believe the fork was committed to but a PR was never opened. If you would like to fix this, please feel free to open a PR! (Or perhaps @elkhadiy is still interested?)

AnOctopus pushed a commit to AnOctopus/mypy that referenced this issue May 1, 2019
Overriding sys.stdout and sys.stderr in mypy.api is not threadsafe.
This causes problems sometimes when using the api in pyls for example.
@AnOctopus
Copy link
Contributor

PR #6750 opened.

gvanrossum pushed a commit that referenced this issue May 15, 2019
Overriding sys.stdout and sys.stderr in mypy.api is not threadsafe.
This causes problems sometimes when using the api in pyls for example.

This started with the changes made by @elkhadiy to fix #6125.

Fixes #6125.
PattenR pushed a commit to PattenR/mypy that referenced this issue Jun 23, 2019
Overriding sys.stdout and sys.stderr in mypy.api is not threadsafe.
This causes problems sometimes when using the api in pyls for example.

This started with the changes made by @elkhadiy to fix python#6125.

Fixes python#6125.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants