GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-28326: Fixes multiprocessing.Process depends on sys.stdout being open by tiagoantao · Pull Request #1410 · python/cpython

Conversation

tiagoantao commented May 3, 2017

Copy link

Copy Markdown

This fixes bug 28326, please see https://bugs.python.org/issue28326 for more details

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          bpo-28326: Fixes multiprocessing.Process depends on sys.stdout being …

71b7abc

…open

mention-bot commented May 3, 2017

Copy link

Copy Markdown

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

the-knights-who-say-ni added the CLA signed label

May 3, 2017

ned-deily requested a review from applio

May 24, 2017 21:09

pitrou commented Jul 22, 2017

Copy link

Copy Markdown

Member

Sorry for not noticing this PR before. Will take a look right now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pitrou reviewed

Jul 22, 2017

View reviewed changes

Comment thread

Lib/test/test_multiprocessing_main_handling.py

		raise RuntimeError("Timed out waiting for results")
		results.sort()
		print(start_method, "->", results)
		sys.stdout.close()

pitrou Jul 22, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

terryjreedy Oct 21, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pitrou commented Jul 22, 2017

Copy link

Copy Markdown

Member

Note you'll need to add a NEWS entry using the blurb tool.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

terryjreedy added skip news and removed skip news labels

Oct 21, 2017

terryjreedy requested changes

Oct 21, 2017

View reviewed changes

terryjreedy left a comment

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/multiprocessing/popen_fork.py

		def __init__(self, process_obj):
		sys.stdout.flush()
		sys.stderr.flush()
		if not sys.stdout.closed:

terryjreedy Oct 21, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_multiprocessing_main_handling.py

		raise RuntimeError("Timed out waiting for results")
		results.sort()
		print(start_method, "->", results)
		sys.stdout.close()

terryjreedy Oct 21, 2017

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot commented Oct 21, 2017

Copy link

Copy Markdown

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 made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting changes label

Oct 21, 2017

tiagoantao commented Oct 23, 2017

Copy link

Copy Markdown

Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pitrou commented Oct 23, 2017

Copy link

Copy Markdown

Member

In any case this was fixed in #4073

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pitrou closed this

Oct 23, 2017

progval mannequin mentioned this pull request

Apr 10, 2022

multiprocessing.Process depends on sys.stdout being open #72513

Closed

Reviewers

pitrou

pitrou left review comments

terryjreedy

terryjreedy requested changes

applio

Awaiting requested review from applio

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.