-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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
bpo-28326: Fixes multiprocessing.Process depends on sys.stdout being open #1410
Conversation
@tiagoantao, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ncoghlan, @berkerpeksag and @shibturn to be potential reviewers. |
Sorry for not noticing this PR before. Will take a look right now. |
@@ -65,6 +65,13 @@ def f(x): | |||
raise RuntimeError("Timed out waiting for results") | |||
results.sort() | |||
print(start_method, "->", results) | |||
sys.stdout.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand it's practical to do so, it's a bit weird to add your test there, as it doesn't really have anything to do with __main__
handling. Instead, I would expect it to be somewhere in _TestProcess
in _test_multiprocessing.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wondered about the placement, so I think you should follow Antoine's suggestion.
Note you'll need to add a NEWS entry using the blurb tool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The news blurb requirement is new since this was submitted. I added and deleted the tag to trigger it.
If you fix the code and test and add a news entry, I think you can also add yourself to Misc/ACKS.
@@ -14,8 +14,10 @@ class Popen(object): | |||
method = 'fork' | |||
|
|||
def __init__(self, process_obj): | |||
sys.stdout.flush() | |||
sys.stderr.flush() | |||
if not sys.stdout.closed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either change this to
if sys.stdout is not None and not sys.stdout.closed:
or change the entire statement to
try:
sys.stdout.flush()
except (AttributeError, ValueError): # stdout None or closed.
pass
and the equivalent for stderr. I slightly prefer the latter (these exceptions should be rare), but if Antoine expresses a preference for the former, use that.
@@ -65,6 +65,13 @@ def f(x): | |||
raise RuntimeError("Timed out waiting for results") | |||
results.sort() | |||
print(start_method, "->", results) | |||
sys.stdout.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wondered about the placement, so I think you should follow Antoine's suggestion.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I have submitted this a long long time ago. I completely lost track of it and have moved on. A suggestion: when you have new people trying to help please be a little bit more responsive. |
In any case this was fixed in #4073 |
This fixes bug 28326, please see https://bugs.python.org/issue28326 for more details