bpo-37193: remove thread objects which finished process its request

[jaraco]

Reverts #23107 (re-submit of #13893)

https://bugs.python.org/issue37193

Automerge-Triggered-By: GH:jaraco

[jaraco]

I was able to replicate the reported failure locally by building a debug version of Python and running with refchecks:

$ ./configure --with-pydebug && make
$ ./python.exe Tools/scripts/run_tests.py -R 3:3 test.test_socketserver

[jaraco]

I've confirmed that the there are leaks in tests not added in this patch, including test_ThreadingUnixStreamServer.

[jaraco]

I've tried:

• removing the lock object on _Threads
• deleting the _threads property on server_close
• other miscellaneous tweaks

But I've been unsuccessful in identifying the cause of the leaks.

@vstinner Do you have any insight into the cause of the leaks?

[jaraco]

I've found that applying this diff stops the memory leaks:

index 6859b69682..751772ef65 100644
--- a/Lib/socketserver.py
+++ b/Lib/socketserver.py
@@ -646,7 +646,7 @@ def remove(self, thread):
         with self._lock:
             # should not happen, but safe to ignore
             with contextlib.suppress(ValueError):
-                super().remove(thread)
+                pass  # super().remove(thread)
 
     def remove_current(self):
         """Remove a current non-daemon thread."""

But it also causes the new test to fail.

[jaraco]

Is it the case that you can't from within a thread remove the last known reference to that thread?

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @jaraco for commit 97c7054 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[jaraco]

This latest commit addresses the issue by leaving the dead threads in the list and reaping them when a new connection is created. It's a little sloppy, meaning that any number of dead threads could be left lying around, but for an active server will not see a continuous growth. It also means that the test is now sloppy, only testing that at least one of the threads got cleaned up. I can't think of another way to more reliably clean up a thread reference from within the thread, short of having another thread responsible for doing the cleanup and signaling from one thread to the other to perform cleanup each time a connection is closed.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @jaraco for commit ec8e689 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit ec8e689 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[pablogsal]

The buildbot/AMD64 Fedora Stable LTO PR and buildbot/PPC64LE Fedora Stable LTO PR can be ignored as the failures are related to some gcc problems:

https://bugs.python.org/issue42164

[github-actions]

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @jaraco for commit ec8e689 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[vstinner]

Should the fix be backported to other branches?

[jaraco]

Yes. Tagging...

[miss-islington]

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

[miss-islington]

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

[bedevere-bot]

GH-24032 is a backport of this pull request to the 3.8 branch.

[bedevere-bot]

GH-24033 is a backport of this pull request to the 3.9 branch.

[miss-islington]

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

[miss-islington]

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

[bedevere-bot]

GH-24749 is a backport of this pull request to the 3.8 branch.

[bedevere-bot]

GH-24750 is a backport of this pull request to the 3.9 branch.