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

bpo-35917: Test multiprocessing manager classes and shareable types #11772

Merged
merged 11 commits into from Feb 7, 2019

Conversation

giampaolo
Copy link
Contributor

@giampaolo giampaolo commented Feb 6, 2019

Related to PR #11664. BPO reference is: https://bugs.python.org/issue35917.

https://bugs.python.org/issue35917

Tests can be run with ./python -m test -v _test_multiprocessing.

@terryjreedy terryjreedy changed the title multiprocessing: provide unittests for manager classes and shareable … bpo-35917: Test multiprocessing manager classes and shareable types Feb 6, 2019
@terryjreedy
Copy link
Member

PR titles must begin with the bpo number when there is one. Note issue-number check to allow merge. This automatically generates link in details and listing of PR on the issue. I changed rest so not too long.

A news item is also required. Use blurb. (Both checks can be over-ridden by labels, but these are used rarely, for things such as typo fixes.)

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

test_multiprocessing_forkserver and test_multiprocessing_spawn fail with a lot of errors here. You might try it for yourself:

./python -m test -m "*Manager*" -v test_multiprocessing_forkserver

Lib/test/_test_multiprocessing.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be poked with soft cushions!

@giampaolo
Copy link
Contributor Author

OK, it appears failures on Windows are fixed.

@pitrou
Copy link
Member

pitrou commented Feb 6, 2019

Hmm, I don't think this is the right solution. You must keep the tests inside _test_multiprocessing, because they are pickled up by all three of test_multiprocessing_{fork,forkserver,spawn}. What you should try is to use classmethods for the process target functions, rather than nested functions.

For example:

    @classmethod
    def _test_event(cls, obj):
        assert obj.is_set()
        obj.wait()
        obj.clear()
        obj.wait(0.001)

    def test_event(self):
        o = self.manager.Event()
        o.set()
        self.run_test(self._test_event, o)
        assert not o.is_set()
        o.wait(0.001)

@pitrou
Copy link
Member

pitrou commented Feb 7, 2019

@giampaolo I think you misunderstood the test failures. A RLock can only be released by the thread/process which acquired it. Example:

>>> import threading                                                                      
>>> lock = threading.RLock()                                                              
>>> lock.acquire()                                                                        
True
>>> t = threading.Thread(target=lock.release)                                             
>>> t.start()                                                                             
Exception in thread Thread-1990:
Traceback (most recent call last):
  File "/home/antoine/miniconda3/envs/pyarrow/lib/python3.7/threading.py", line 917, in _bootstrap_inner
    self.run()
  File "/home/antoine/miniconda3/envs/pyarrow/lib/python3.7/threading.py", line 865, in run
    self._target(*self._args, **self._kwargs)
RuntimeError: cannot release un-acquired lock

Same with multiprocessing:

>>> lock = mp.RLock()                                                                     
>>> lock.acquire()                                                                        
True
>>> proc = mp.Process(target=lock.release)                                                
>>> proc.start()                                                                          
Process Process-1:
>>> Traceback (most recent call last):                                                    
  File "/home/antoine/miniconda3/envs/pyarrow/lib/python3.7/multiprocessing/process.py", line 297, in _bootstrap
    self.run()
  File "/home/antoine/miniconda3/envs/pyarrow/lib/python3.7/multiprocessing/process.py", line 99, in run
    self._target(*self._args, **self._kwargs)
AssertionError: attempt to release recursive lock not owned by thread

So you should write the RLock and Condition tests differently.

@giampaolo
Copy link
Contributor Author

Thanks Antoine. That was the problem indeed (got tricked by the fact that for some reason Linux behaves differently). Good to merge?

@pitrou
Copy link
Member

pitrou commented Feb 7, 2019

It's actually a bit weird that Linux behaves differently. Can you open an issue for that?

@pitrou pitrou added needs backport to 3.7 tests Tests in the Lib/test dir labels Feb 7, 2019
@pitrou pitrou merged commit 2848d9d into master Feb 7, 2019
@miss-islington
Copy link
Contributor

Thanks @giampaolo for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @giampaolo and @pitrou, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2848d9d29914948621bce26bf0d9a89f2e19b97b 3.7

@giampaolo giampaolo deleted the mp-manager-tests branch February 7, 2019 11:08
@bedevere-bot
Copy link

GH-11780 is a backport of this pull request to the 3.7 branch.

pitrou pushed a commit to pitrou/cpython that referenced this pull request Feb 7, 2019
…ypes (pythonGH-11772)

multiprocessing: provide unittests for manager classes and shareable types.
(cherry picked from commit 2848d9d)

Co-authored-by: Giampaolo Rodola <g.rodola@gmail.com>
@giampaolo
Copy link
Contributor Author

giampaolo commented Feb 7, 2019

Will do. Do you think we should backport this to 3.7 only? Or also for previous releases?

@pitrou
Copy link
Member

pitrou commented Feb 7, 2019

3.7 only. Backporting to 2.7 would probably be more involved.

pitrou added a commit that referenced this pull request Feb 7, 2019
…ypes (GH-11772) (GH-11780)

multiprocessing: provide unittests for manager classes and shareable types.
(cherry picked from commit 2848d9d)

Co-authored-by: Giampaolo Rodola <g.rodola@gmail.com>
@pablogsal
Copy link
Member

@pitrou @giampaolo

This PR broke some buildbots with refleak checks because the tests modify the environment (leaking processes or threads or files ...). Some of the failures:

https://buildbot.python.org/all/#builders/80/builds/506
ttps://buildbot.python.org/all/#builders/114/builds/375
https://buildbot.python.org/all/#builders/1/builds/497

I opened https://bugs.python.org/issue35940 to track this.

I think I can find some time tonight to check if I am able to find out where the problem is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants