bpo-37193: remove thread objects which finished process its request#23127
bpo-37193: remove thread objects which finished process its request#23127
Conversation
|
I was able to replicate the reported failure locally by building a debug version of Python and running with refchecks: |
|
I've confirmed that the there are leaks in tests not added in this patch, including |
|
I've tried:
But I've been unsuccessful in identifying the cause of the leaks. @vstinner Do you have any insight into the cause of the leaks? |
|
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. |
|
Is it the case that you can't from within a thread remove the last known reference to that thread? |
|
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. |
|
🤖 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. |
|
The https://bugs.python.org/issue42164 |
|
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 |
|
Should the fix be backported to other branches? |
|
Yes. Tagging... |
|
Thanks @jaraco for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
Thanks @jaraco for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
|
GH-24032 is a backport of this pull request to the 3.8 branch. |
|
GH-24033 is a backport of this pull request to the 3.9 branch. |
…ythonGH-23127) This reverts commit aca67da. (cherry picked from commit b5711c9) Co-authored-by: Jason R. Coombs <[email protected]>
…ythonGH-23127) This reverts commit aca67da. (cherry picked from commit b5711c9) Co-authored-by: Jason R. Coombs <[email protected]>
|
Thanks @jaraco for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
Thanks @jaraco for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
|
GH-24749 is a backport of this pull request to the 3.8 branch. |
…ythonGH-23127) This reverts commit aca67da. (cherry picked from commit b5711c9) Co-authored-by: Jason R. Coombs <[email protected]>
|
GH-24750 is a backport of this pull request to the 3.9 branch. |
…ythonGH-23127) This reverts commit aca67da. (cherry picked from commit b5711c9) Co-authored-by: Jason R. Coombs <[email protected]>
…uest (GH-23127) (GH-24750) This reverts commit aca67da. (cherry picked from commit b5711c9) Co-authored-by: Jason R. Coombs <[email protected]> Automerge-Triggered-By: GH:jaraco
…uest (GH-23127) (GH-24749) This reverts commit aca67da. (cherry picked from commit b5711c9) Co-authored-by: Jason R. Coombs <[email protected]> Automerge-Triggered-By: GH:jaraco
…ythonGH-23127) This reverts commit aca67da.
Reverts #23107 (re-submit of #13893)
https://bugs.python.org/issue37193
Automerge-Triggered-By: GH:jaraco