Skip to content

Fix sys.stdout overriding in mypy.api #6750

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
merged 4 commits into from
May 15, 2019

Conversation

AnOctopus
Copy link
Contributor

@AnOctopus AnOctopus commented May 1, 2019

These are the changes made by @elkhadiy to resolve the problems with stdout not being thread-safe in the api.

Fixes #6125.

@gvanrossum gvanrossum changed the title Fix sys.stdout overriding in mypy.api (#6125) Fix sys.stdout overriding in mypy.api May 1, 2019
@gvanrossum
Copy link
Member

Sadly this is not enough. When -v is specified, build.py writes directly to sys.stderr. There are probably also other places where sys.stderr is written to directly. (And possibly even places where sys.stdout is implicitly used via print() calls without file=....)

For example, I have this simple test program:

import mypy.api

stdout, stderr, exitcode = mypy.api.run(['-v', '_.py'])
print('=== stdout ===')
print(stdout)
print('=== stderr ===')
print(stderr)
print('=== exit %d ===' % exitcode)

Note that without your patch, the LOG entries are printed under the === stderr === heading. But with your patch, they precede the === stdout === header.

@AnOctopus AnOctopus force-pushed the fix_stdout_hijacking branch 3 times, most recently from a46c4f3 to bc4e441 Compare May 14, 2019 14:06
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 AnOctopus force-pushed the fix_stdout_hijacking branch from bc4e441 to 24f78eb Compare May 14, 2019 14:22
@gvanrossum
Copy link
Member

PS. While a PR is under review, please don't force-push. This makes the life difficult for the reviewer. We always squash commits when we merge them into master.

Is this now ready for another review?

@AnOctopus
Copy link
Contributor Author

I realized the issue with it when I read through the contributing guidelines a bit more thoroughly and saw it said to not squash them on my end. I believe it's ready for review. It's possible that I missed something in my fixes or that things were messed up when I rebased to the current master, but I think I got all the code reachable from the api, and the simple test program you provided has the expected output.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I think this is much better. Now we just have to figure out a way not to default to the values of sys.stdout/stderr set at module load time...

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks. This looks great! I'll merge this now.

@gvanrossum gvanrossum merged commit faebf3c into python:master May 15, 2019
PattenR pushed a commit to PattenR/mypy that referenced this pull request 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stdout hijacking in api.py is not thread safe
3 participants