classification
Title: Fix test discovery for test_concurrent_futures.py
Type: behavior Stage: patch review
Components: Tests Versions: Python 3.5, Python 3.4
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, chris.jerdonek, ezio.melotti, martin.panter, serhiy.storchaka, zach.ware
Priority: normal Keywords: patch

Created on 2013-01-14 22:19 by zach.ware, last changed 2016-08-21 08:36 by BreamoreBoy.

Files
File name Uploaded Description Edit
test_concurrent_futures_discovery.diff zach.ware, 2013-01-14 22:19 Fix discovery, but not test_main review
test_concurrent_futures_discovery.v2.diff zach.ware, 2013-01-24 22:30 v2, test_main replaced but gives warning review
test_concurrent_futures_discovery.v3.diff zach.ware, 2013-01-28 15:29 Version 3 review
test_concurrent_futures_discovery.v4.diff zach.ware, 2013-01-29 17:50 Version 4 review
test_concurrent_futures_discovery.v5.diff zach.ware, 2013-08-08 03:29 Version 5
Messages (23)
msg179983 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-01-14 22:19
Here's an incomplete patch to test_concurrent_futures.py that does not convert from test_main() to unittest.main(); the decorator on test_main has me unsure how to make the conversion.  I've attempted moving various parts of the decorator's functions to setUpModule and tearDownModule functions, but always wind up with a warning that the test has modified threading._dangling.

Guidance on this one would be much appreciated :)
msg179984 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-01-14 22:28
The way I would replace test_main() would be:

if __name__ == '__main__':
  try:
    test.support.reap_threads(unittest.main)()
  finally:
    test.support_reap_children()
msg179985 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-01-14 22:40
That looks quite good, except for the fact that discovery (of the form `python -m unittest discover Lib/test/ 'test_*.py'`) won't hit reap_threads or reap_children.  It does cover `python -m test.test_concurrent_futures` and standard regrtest, though; is 2 out of three good enough?
msg179992 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-01-14 23:20
Yes as even the current solution doesn't work with ``-m unittest discover`` anyway, right? And if you are running the tests independently then it is a separate process and thus you don't need to worry about dangling threads or processes.
msg179993 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-01-14 23:28
Ok then, I'll update the patch. Thanks, Brett!
msg179997 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-15 02:31
If you want to get the same cleanup logic working with unittest discovery, you could try using the load_tests protocol, and wrapping the functions that actually run the tests there (which may simply mean wrapping the tests passed to load_tests, which I believe are callables).
msg179998 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-15 02:36
By the way, I think this process of using unittest (i.e. dog-fooding) is a good exercise in part because it helps us understand better where unittest could use improvement (e.g. the SkipTest during import issue, and the decorator issue raised here).
msg180041 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-01-15 18:16
Hmmm, actually, I was wrong; Brett's suggestion doesn't cover running as part of regrtest.  It looks like Chris is right and some kind of load_tests magic will have to happen.  I'm looking into it now.
msg180551 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-01-24 22:30
I'm at a loss on this one.  I tried a few different methods of wrapping the tests in load_tests(), I tried doing support.threading_setup() in setUpModule and support.threading_cleanup() in tearDownModule, tried the same in each test class (following the example of test_asynchat.TestAsynchat).  Every way I've tried, I still get the warning that threading._dangling was modified when running the test as part of regrtest, even though it appears that everything is getting called at the right time with the right arguments.

I did just stumble onto something interesting: if you simply remove the decorator and finally clause from test_main and run it (with the initial discovery patch applied), there is no warning at the end.  Remove test_main and there's the warning.  Add a load_tests that builds a suite in what appears to be the same order as what support.run_unittest does, and the warning is still there.  I can't find what makes the difference.

The attached diff is the best I've come up with that seems to do the right things with the reap functions, but still causes the warning on my machine.

Am I missing something obvious here or what?
msg180566 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-25 10:58
I looked into this a bit.  It seems like this is because with the patch, the call to "loader.loadTestsFromModule(the_module)" inside regrtest comes before the try-finally:

http://hg.python.org/cpython/file/fcdb35b114ab/Lib/test/regrtest.py#l1277

whereas with the current code, the analogous test-loading code is part of test.support.run_unittest() and so is protected by the try-finally inside test_main().  Apparently, simply discovering/loading tests from test_concurrent_futures.py is enough to modify threading._dangling (e.g. when finding the tests to pass to load_tests).

I'm not sure yet what the right solution is, but it doesn't seem like test discovery should have that side effect.  It could be because of how test_concurrent_futures is written, or because of certain initialization code in one of the modules it depends on.
msg180567 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-25 11:13
I confirmed the above by changing regrtest as follows in the patch (moving the decorator from load_tests()), and trying both placements of the loadTestsFromModule() line.  One gives the warning and one does not:

    if test_runner is None:
        loader = unittest.TestLoader()
        #tests = loader.loadTestsFromModule(the_module)
        @support.reap_threads
        def test_runner():
            try:
                tests = loader.loadTestsFromModule(the_module)
                support.run_unittest(tests)
            finally:
                support.reap_children()
    test_runner()

This is mostly a suggestion though -- not 100%.
msg180689 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-26 17:22
Okay, I think I understand the issue better now.  The threading._dangling warning happens because when leaving the saved_test_environment context manager in regrtest:

http://hg.python.org/cpython/file/fcdb35b114ab/Lib/test/regrtest.py#l1271

the context manager finds threads still in threading._dangling that weren't there at the beginning.  I think the threads are still in there because the "tests" variable in the code linked to above is holding references to those threads (which isn't surprising given what test_concurrent_futures.py tests).  So this is preventing the threads (which are in a WeakSet) from being garbage collected.

On my machine, severing those references before leaving the context manager solves the problem.  You can do this by adding "test = 0" after the test_runner() line, or defining "tests" inside an inner function as follows:

if test_runner is None:
    def test_runner():
        tests = unittest.TestLoader().loadTestsFromModule(the_module)
        support.run_unittest(tests)
test_runner()

To be completely sure, we may also need to do something to ensure that the threads are garbage collected before leaving the context manager.  Currently, an explicit call to support.gc_collect() happens in threading_cleanup() (which is in turn called by reap_threads(), etc):

http://hg.python.org/cpython/file/fcdb35b114ab/Lib/test/support.py#l1665

But I'm not sure if that's late enough in the process to guarantee garbage collection of those threads with the test_runner() change above (since the tests list might still be defined).
msg180860 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-01-28 15:29
Thank you, Chris.  I'm rather ashamed of how long I've spent beating my head on this issue and missed the spare tests reference in runtest_inner.

Simply removing the "tests" name entirely clears things up, if this isn't too ugly:

diff -r 5f655369ef06 Lib/test/regrtest.py
--- a/Lib/test/regrtest.py      Mon Jan 28 13:27:02 2013 +0200
+++ b/Lib/test/regrtest.py      Mon Jan 28 08:50:59 2013 -0600
@@ -1275,8 +1275,8 @@
             # tests.  If not, use normal unittest test loading.
             test_runner = getattr(the_module, "test_main", None)
             if test_runner is None:
-                tests = unittest.TestLoader().loadTestsFromModule(the_module)
-                test_runner = lambda: support.run_unittest(tests)
+                test_runner = lambda: support.run_unittest(
+                        unittest.TestLoader().loadTestsFromModule(the_module))
             test_runner()
             if huntrleaks:
                 refleak = dash_R(the_module, test, test_runner,

As far as the reap_threads wrapper and reap_children follow-up, I think the TestSuite subclass and load_tests function in the last patch I uploaded may be about the simplest way to keep them for this test without adding them to all tests (by adding it to regrtest.runtest_inner).  If anyone thinks the 'ReapedSuite' class (or a better named copy) could be useful elsewhere, it might could go in test.support which would make test_concurrent_futures look a little cleaner.

Patch v3 is v2 plus the regrtest change inline above.
msg180876 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-28 19:37
Whichever solution you pick for the "test" issue, I would at least add a code comment explaining that the test return value needs to be garbage collected and why, etc. and probably reference this issue.

> If anyone thinks the 'ReapedSuite' class (or a better named copy) could be useful elsewhere,

Can you look at some of the other test modules' test_main() functions to see if the same pattern is used there (or look at other uses of reap_children(), etc)?  Then we would know what would be useful elsewhere.
msg180917 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-01-29 17:50
Right you are, Chris.  v4 has a comment added to regrtest.runtest_inner pointing back to this issue.

Also in v4, ReapedSuite has been moved to test.support.  At least one other test module (test_pydoc) uses the same idiom as test_concurrent_futures, and so could use this suite for the same effect.  Several others use at least one of reap_threads (or its component pieces) or reap_children in test_main, and I believe those could also use ReapedSuite for simplicity's sake.  Used in its current form, it shouldn't cause any issues other than perhaps an extra couple of function calls.
msg180950 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-01-29 21:33
I failed to mention; v4 also removes setUpModule() which was present in the first 3 patches.  With 16935 fixed, setUpModule would be unnecessary.
msg186531 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-04-10 21:14
Ping.

Version 4 still applies cleanly, and with issue16935 fixed, it works properly.  The fix for test_socket is looking like it could use the ReapedSuite from this patch, so would anyone mind looking at this one again and committing if it's qualified?

Thanks,

Zach
msg194533 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-06 11:50
As far as I understand threads reaping needed only when ThreadPoolExecutor is used and children reaping needed only when ProcessPoolExecutor is used. Why not use them only in tests which needs them?

class ThreadPoolMixin(ExecutorMixin):
    executor_type = futures.ThreadPoolExecutor

    def run(self, result):
        key = test.support.threading_setup()
        try:
            return super().run(result)
        finally:
            test.support.threading_cleanup(*key)


class ProcessPoolMixin(ExecutorMixin):
    executor_type = futures.ProcessPoolExecutor

    def run(self, result):
        try:
            return super().run(result)
        finally:
            test.support.reap_children()
msg194643 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-08-08 03:29
That's a much better solution, thank you, Serhiy.  Here's a new patch.  test.support is no longer changed at all, but regrtest.py still is; the extra reference to the tests still causes issues, so it is removed.
msg194910 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-11 20:12
I think this problem deserves a discussion on Python-Dev: http://comments.gmane.org/gmane.comp.python.devel/141068 .
msg222058 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-07-01 21:12
I followed the link in msg194910 to python-dev but didn't find a single response so where does that leave us?  I ask because I'd like to help out with the outstanding test issues, but there are so many of them and they seem so interlinked that I don't know where to start.  Please advise.
msg222107 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-07-02 14:23
Since then, Senthil Kumaran converted test_urllib2_localnet (and another urllib test or two) to unittest.main, using this:

"""
threads_key = None

def setUpModule():
    # Store the threading_setup in a key and ensure that it is cleaned up
    # in the tearDown
    global threads_key
    threads_key = support.threading_setup()

def tearDownModule():
    if threads_key:
        support.threading_cleanup(threads_key)

if __name__ == "__main__":
    unittest.main()
"""

Since nobody has said anything against that in the two months since it was done, that's what I was going to go with when I got back to this; support.reap_children() can just be called in tearDownModule.  If you'd like to provide a patch, Mark, please do so!
msg273260 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-08-21 02:50
I don’t know much about the concurrent.futures testing, but in general IMO it makes more sense to call thread.join(), or at least @reap_threads, in each individual test case that needs it. If appropriate, you can call join() with a one-second timeout, which should be functionally equivalent to @reap_threads. Leaving a background thread running while you start another test seems like a bad idea; concurrent tests aren’t meant to be run in the same process.

Also, I added a gc_collect() call to the test infrastructure in Issue 27787, which may help avoid problems with spurious “dangling” thread object references.
History
Date User Action Args
2016-08-21 08:36:36BreamoreBoysetnosy: - BreamoreBoy
2016-08-21 02:50:28martin.pantersetnosy: + martin.panter
messages: + msg273260
2014-07-02 14:23:26zach.waresetmessages: + msg222107
versions: + Python 3.5, - Python 3.3
2014-07-01 21:12:42BreamoreBoysetnosy: + BreamoreBoy
messages: + msg222058
2013-12-17 21:26:38zach.warelinkissue16748 dependencies
2013-08-11 20:12:33serhiy.storchakasetmessages: + msg194910
2013-08-08 03:29:38zach.waresetfiles: + test_concurrent_futures_discovery.v5.diff

messages: + msg194643
2013-08-06 11:50:06serhiy.storchakasetmessages: + msg194533
2013-07-17 11:48:07serhiy.storchakasetnosy: + serhiy.storchaka

stage: patch review
2013-04-16 20:52:36r.david.murraylinkissue14408 dependencies
2013-04-10 21:14:49zach.waresetmessages: + msg186531
2013-01-29 21:33:29zach.waresetmessages: + msg180950
2013-01-29 17:50:11zach.waresetfiles: + test_concurrent_futures_discovery.v4.diff

messages: + msg180917
2013-01-28 19:37:24chris.jerdoneksetmessages: + msg180876
2013-01-28 15:29:40zach.waresetfiles: + test_concurrent_futures_discovery.v3.diff

messages: + msg180860
2013-01-26 17:22:48chris.jerdoneksetmessages: + msg180689
2013-01-25 11:13:35chris.jerdoneksetmessages: + msg180567
2013-01-25 10:58:10chris.jerdoneksetmessages: + msg180566
2013-01-24 22:30:50zach.waresetfiles: + test_concurrent_futures_discovery.v2.diff

messages: + msg180551
2013-01-15 18:16:11zach.waresetmessages: + msg180041
2013-01-15 02:36:44chris.jerdoneksetmessages: + msg179998
2013-01-15 02:31:06chris.jerdoneksetnosy: + chris.jerdonek
messages: + msg179997
2013-01-14 23:28:35zach.waresetmessages: + msg179993
2013-01-14 23:20:18brett.cannonsetmessages: + msg179992
2013-01-14 22:40:12zach.waresetmessages: + msg179985
2013-01-14 22:28:56brett.cannonsetmessages: + msg179984
2013-01-14 22:19:34zach.warecreate