bpo-31804: multiprocessing calls flush on sys.stdout at exit even if … #5492

SlaushVunter · 2018-02-02T12:46:43Z

…it is None (pythonw)

If you start Python by pythonw.exe on Windows platform then sys.stdout and sys.stderr are set to None. If you also use multiprocessing then when the child process finishes BaseProcess._bootstrap calls sys.stdout.flush() and sys.stderr.flush() finally. This causes the process return code to be not zero (it is 1).

https://bugs.python.org/issue31804

…it is None (pythonw) If you start Python by pythonw.exe on Windows platform then sys.stdout and sys.stderr are set to None. If you also use multiprocessing then when the child process finishes BaseProcess._bootstrap calls sys.stdout.flush() and sys.stderr.flush() finally. This causes the process return code to be not zero (it is 1).

the-knights-who-say-ni · 2018-02-02T12:46:45Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

pitrou · 2018-02-02T17:47:02Z

Thanks for posting this. Before going forward, you'll need to do two things:

sign the CLA as explained by the bot above
add a NEWS file which you can generate using the blurb utility.

I'll also post some comments on your PR.

pitrou · 2018-02-02T17:47:44Z

Lib/multiprocessing/process.py

@@ -314,8 +314,10 @@ def _bootstrap(self):
        finally:
            threading._shutdown()
            util.info('process exiting with exitcode %d' % exitcode)
-            sys.stdout.flush()
-            sys.stderr.flush()
+            if sys.stdout is not None and not sys.stdout.closed:


A minor issue, but I'd like to stick to the idiom already used in the previous PR: https://github.com/python/cpython/pull/4073/files#diff-d6a41af14d72f372f37ced0cc15c1f58L17

Of course. It's the easier to ask for forgiveness than permission idiom.

pitrou · 2018-02-02T17:50:09Z

Lib/test/_test_multiprocessing.py

@@ -653,6 +653,47 @@ def test_forkserver_sigkill(self):
            self.check_forkserver_death(signal.SIGKILL)


+class TestStdOutAndErr(unittest.TestCase):


Perhaps you can move these tests closer to the test added by the other PR:
https://github.com/python/cpython/pull/4073/files#diff-b046ab474480855fb4a01de88cfc82bbR585

I also wonder if those tests can be unified. They seem to be doing very similar things.

My 1st guess was to update that test case too. But someone has disabled that test case by adding a _ prefix to the class name. Also the test module itself is prefixed by _. So these tests are not run automatically. And I couldn't make that test case work.
Also that test seemed to test the inter process Event handling, so I guessed I would stick to the strict and often unpractical "only test one thing at a time" principle.
I think the "_test_multiprocess.py" modul should be refurbished but I don't think that I am the right person for that.

Ha. You misunderstood how _test_multiprocessing works :-) The test classes there are used as base classes for the actual tests in test_multiprocessing_fork, test_multiprocessing_forkserver and test_multiprocessing_spawn. So you just have to run one of those three test modules to get the tests to work!

(and that's why those test classes have an underscore prefix)

Whoa! :) Now it all makes sense! I will make yet another commit (3rd) and unify the test cases.

pitrou · 2018-02-02T17:51:08Z

Lib/test/_test_multiprocessing.py

+
+    def test_no_stdio(self):
+        """
+        bpo-31804: If you start Python by pythonw then sys.stdout and


I think there's no need for the docstring (or comment) to be that long. Just put the reference to the issue number + a 2- or 3-line summary.

I trimmed the docstring.

bedevere-bot · 2018-02-02T17:51:47Z

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.

…it is None (pythonw.exe) - Review changes

SlaushVunter · 2018-02-12T11:14:00Z

I have made the requested changes; please review again

bedevere-bot · 2018-02-12T11:14:03Z

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

…dd close method to _file_like

SlaushVunter · 2018-02-22T15:47:01Z

I unified the unit tests. I recognized that the previous test did not test the spawn method correctly, because it set the sys.stdout and 'sys.stderr' before creating the child process and assumed that the child process will inherit this. Well not with the spawn method (on Windows at least). Maybe it worked so in the past? In particular the spawned process inherits the sys.__stderr__ and not the sys.stderr (I guess the same is true for stdout). You may check this with the following snippet:

import sys

import tempfile
import multiprocessing

def print_stderr():
    print('stderr:', sys.stderr.fileno(), sys.stderr)
    print('__stderr__:', sys.__stderr__.fileno(), sys.__stderr__)

if __name__ == '__main__':
    sys.stderr = tempfile.TemporaryFile('w')

    p = multiprocessing.Process(target=print_stderr)
    print('Parent:')
    print_stderr()
    print('Child:')
    p.start()
    p.join()

According to the documentation of multiprocessing:

The child process will only inherit those resources necessary to run the process objects run() method. In particular, unnecessary file descriptors and handles from the parent process will not be inherited.

I'm not so sure what unnecessary means but I have read that inheriting file descriptors can pose a security risk. Can this be the reason why custom outputs are not inherited? Or is this another bug?

Anyhow I replaced the similar test with my test because mine changes the stdout and stderr in the child processes so it does not depend on this, then extended my test with the Event check. Also the plain StringIO as a substitute for stdout and stderr is not good enough because it's flush method doesn't raise an exception if it was closed before.

I have made the requested changes; please review again

bedevere-bot · 2018-02-22T15:47:04Z

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

pitrou · 2018-03-11T17:35:20Z

@poxthegreat thanks for the updates! Actually we need to test both cases: the one where stdout/stderr are mutated before launching the process, and the one where they are mutated inside the child process. I'll produce a new PR with the required changes.

pitrou · 2018-03-11T18:20:43Z

I'm closing this PR in favor of PR #6079.

the-knights-who-say-ni added the CLA not signed label Feb 2, 2018

bedevere-bot added the awaiting review label Feb 2, 2018

pitrou added needs backport to 3.6 labels Feb 2, 2018

pitrou requested changes Feb 2, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Feb 2, 2018

bpo-31804: multiprocessing calls flush on sys.stdout at exit even if …

fe03c62

…it is None (pythonw.exe) - Review changes

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Feb 12, 2018

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 12, 2018

SlaushVunter added 2 commits February 22, 2018 14:41

bpo-31804: Review changes 2 - unit tests unified

e860a4f

bpo-31804: Review changes 3 - decorate StringIO with _file_like and a…

5e91e60

…dd close method to _file_like

pitrou closed this Mar 11, 2018

serhiy-storchaka removed needs backport to 3.6 labels Mar 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31804: multiprocessing calls flush on sys.stdout at exit even if … #5492

bpo-31804: multiprocessing calls flush on sys.stdout at exit even if … #5492

SlaushVunter commented Feb 2, 2018 •

edited by bedevere-bot

Loading

the-knights-who-say-ni commented Feb 2, 2018

pitrou commented Feb 2, 2018

pitrou Feb 2, 2018

SlaushVunter Feb 12, 2018

pitrou Feb 2, 2018

SlaushVunter Feb 12, 2018

pitrou Feb 12, 2018 •

edited

Loading

SlaushVunter Feb 14, 2018

pitrou Feb 2, 2018

SlaushVunter Feb 12, 2018

bedevere-bot commented Feb 2, 2018

SlaushVunter commented Feb 12, 2018

bedevere-bot commented Feb 12, 2018

SlaushVunter commented Feb 22, 2018

bedevere-bot commented Feb 22, 2018

pitrou commented Mar 11, 2018

pitrou commented Mar 11, 2018

		@@ -653,6 +653,47 @@ def test_forkserver_sigkill(self):
		self.check_forkserver_death(signal.SIGKILL)


		class TestStdOutAndErr(unittest.TestCase):

bpo-31804: multiprocessing calls flush on sys.stdout at exit even if … #5492

bpo-31804: multiprocessing calls flush on sys.stdout at exit even if … #5492

Conversation

SlaushVunter commented Feb 2, 2018 • edited by bedevere-bot Loading

the-knights-who-say-ni commented Feb 2, 2018

pitrou commented Feb 2, 2018

pitrou Feb 2, 2018

Choose a reason for hiding this comment

SlaushVunter Feb 12, 2018

Choose a reason for hiding this comment

pitrou Feb 2, 2018

Choose a reason for hiding this comment

SlaushVunter Feb 12, 2018

Choose a reason for hiding this comment

pitrou Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

SlaushVunter Feb 14, 2018

Choose a reason for hiding this comment

pitrou Feb 2, 2018

Choose a reason for hiding this comment

SlaushVunter Feb 12, 2018

Choose a reason for hiding this comment

bedevere-bot commented Feb 2, 2018

SlaushVunter commented Feb 12, 2018

bedevere-bot commented Feb 12, 2018

SlaushVunter commented Feb 22, 2018

bedevere-bot commented Feb 22, 2018

pitrou commented Mar 11, 2018

pitrou commented Mar 11, 2018

SlaushVunter commented Feb 2, 2018 •

edited by bedevere-bot

Loading

pitrou Feb 12, 2018 •

edited

Loading