Issue 45212: Dangling threads in skipped tests in test_socket

Dangling threads are reported when run test_socket tests which raise SkipTest in setUp() in refleak mode.

$ ./python -m test -R 3:3 test_socket -m testBCM
0:00:00 load avg: 2.53 Run tests sequentially
0:00:00 load avg: 2.53 [1/1] test_socket
beginning 6 repetitions
123456
.Warning -- threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 1)
Warning -- Dangling thread: <_MainThread(MainThread, started 139675429708416)>
.Warning -- threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 1)
Warning -- Dangling thread: <_MainThread(MainThread, started 139675429708416)>
.Warning -- threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 1)
Warning -- Dangling thread: <_MainThread(MainThread, started 139675429708416)>
...
test_socket failed (env changed)

== Tests result: SUCCESS ==

1 test altered the execution environment:
    test_socket

Total duration: 655 ms
Tests result: SUCCESS


It happens because tearDown() is not called if setUp() raises any exception (including SkipTest). If we want to execute some cleanup code it should be registered with addCleanup().

New changeset 7dacb70485a0910eb298c24b4d051720ca56fb91 by Serhiy Storchaka in branch 'main':
bpo-45212: Fix dangling threads in skipped tests in test_socket (GH-28361)
https://github.com/python/cpython/commit/7dacb70485a0910eb298c24b4d051720ca56fb91

def testWithTimeoutTriggeredSend(self):
        conn = self.accept_conn()
        conn.recv(88192)
+        time.sleep(1)

Was it a deliberate choice to add a sleep of 1 second? If yes, can you please add a comment to explain why?

Yes, it is a deliberate choice. It is difficult to explain why it was passed without it -- dues to a specific order of calling tearDown() and callbacks registered with addCleanup() in different base classes.

Using an event object would fix it too, but:

* Re-using an existing event object (like self.done) can have side effects in future.
* Introducing a new event object has a risk of name conflicts, because the hierarchy of classes is complicated, and same name can be reused by accident.
* time.sleep() is already used in other similar tests for timeouts. We only need to do a sleep longer then the timeout on the client side (0.01).

Given all this, I decided that adding time.sleep(1) was the simplest, the most obvious, and maybe the most resistant to future human errors way. If we want to use an event object here, we will need to re-consider using time.sleep() in other tests.

What comment do you want to add?

New changeset ce59ac93626004894c2b291ec599a36cfa9fb0be by Serhiy Storchaka in branch '3.10':
[3.10] bpo-45212: Fix dangling threads in skipped tests in test_socket (GH-28361) (GH-28409)
https://github.com/python/cpython/commit/ce59ac93626004894c2b291ec599a36cfa9fb0be

New changeset 10c3cf78518f4b31e1527c2795694b1bcb092696 by Serhiy Storchaka in branch '3.9':
[3.9] bpo-45212: Fix dangling threads in skipped tests in test_socket (GH-28361) (GH-28408)
https://github.com/python/cpython/commit/10c3cf78518f4b31e1527c2795694b1bcb092696

We can add a comment to the effect that "the wait here needs to be longer than the client-side (0.01s)". Since you already merged the change to `main`, it will be easiest to merge the backports as well and add the comment in a follow-up series.

Haha, which you just did. Cool.

I have merged the backported PRs because they are blockers for my other PR which is a blocker for my other PR which is a blocker for my other PR and several future PRs.

New changeset 54a1760cde7bb01e5574734c389c0746762218fd by Serhiy Storchaka in branch 'main':
bpo-45212: Add a comment for time.sleep() in tests (GH-28414)
https://github.com/python/cpython/commit/54a1760cde7bb01e5574734c389c0746762218fd