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 :)

The way I would replace test_main() would be:

if __name__ == '__main__':
  try:
    test.support.reap_threads(unittest.main)()
  finally:
    test.support_reap_children()

That looks quite good, except for the fact that discovery (of the form python -m unittest discover [Lib/test/](https://github.com/python/cpython/blob/main/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?

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.

Ok then, I'll update the patch. Thanks, Brett!

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

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

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.

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?

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.

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

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

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.

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.

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.

I failed to mention; v4 also removes setUpModule() which was present in the first 3 patches. With 16935 fixed, setUpModule would be unnecessary.

Ping.

Version 4 still applies cleanly, and with bpo-16935 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

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()

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.

I think this problem deserves a discussion on Python-Dev: http://comments.gmane.org/gmane.comp.python.devel/141068 .

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.

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!

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 bpo-27787, which may help avoid problems with spurious “dangling” thread object references.

Looking at this again after 5 years, things have changed a bit and I also better understand what's actually going on in the tests. IIUC, the test_main function's only real purpose was to cause any dangling threads or processes to be reaped, which should actually be done by the shutdown methods of the tested executors. Doing that cleanup in test_main (if successful), would actually prevent us from getting useful warnings from regrtest if shutdown did not clean up properly, so the attached PR simply removes test_main.

Seems that reap_children() in regrtest is ran only if the test is passed. And it doesn't produce warnings.

test_concurrent_futures was made discoverable by Victor Stinner in bpo-37421. It used tearDownModule() to run threading_cleanup() and reap_children().

test_concurrent_futures was made discoverable by Victor Stinner in bpo-37421. It used tearDownModule() to run threading_cleanup() and reap_children().

Sounds like we can close this, then.

Fixed by gh-14563 in gh-81602.