Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
pablogsal merged 6 commits into
masterpython/cpython:masterfrom
revert-23107-revert-13893-fix-issue-37193python/cpython:revert-23107-revert-13893-fix-issue-37193Copy head branch name to clipboard
Dec 31, 2020
Merged

bpo-37193: remove thread objects which finished process its request#23127
pablogsal merged 6 commits into
masterpython/cpython:masterfrom
revert-23107-revert-13893-fix-issue-37193python/cpython:revert-23107-revert-13893-fix-issue-37193Copy head branch name to clipboard

Conversation

@jaraco

@jaraco jaraco commented Nov 3, 2020

Copy link
Copy Markdown
Member

Reverts #23107 (re-submit of #13893)

https://bugs.python.org/issue37193

Automerge-Triggered-By: GH:jaraco

@jaraco

jaraco commented Nov 3, 2020

Copy link
Copy Markdown
Member Author

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

jaraco commented Nov 3, 2020

Copy link
Copy Markdown
Member Author

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

@jaraco

jaraco commented Nov 3, 2020

Copy link
Copy Markdown
Member Author

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

jaraco commented Nov 4, 2020

Copy link
Copy Markdown
Member Author

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

jaraco commented Nov 4, 2020

Copy link
Copy Markdown
Member Author

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

@jaraco jaraco added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 4, 2020
@bedevere-bot

Copy link
Copy Markdown

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 4, 2020
@jaraco

jaraco commented Nov 4, 2020

Copy link
Copy Markdown
Member Author

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.

@jaraco jaraco requested a review from vstinner November 4, 2020 01:42
@jaraco jaraco marked this pull request as ready for review November 4, 2020 01:42
@jaraco jaraco added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 4, 2020
@bedevere-bot

Copy link
Copy Markdown

🤖 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 bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 4, 2020
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 4, 2020
@bedevere-bot

Copy link
Copy Markdown

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 4, 2020
@pablogsal

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown

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

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Dec 16, 2020
@jaraco jaraco added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 16, 2020
@bedevere-bot

Copy link
Copy Markdown

🤖 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 bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 16, 2020
@pablogsal pablogsal merged commit b5711c9 into master Dec 31, 2020
@pablogsal pablogsal deleted the revert-23107-revert-13893-fix-issue-37193 branch December 31, 2020 20:19
@vstinner

Copy link
Copy Markdown
Member

Should the fix be backported to other branches?

@jaraco

jaraco commented Jan 1, 2021

Copy link
Copy Markdown
Member Author

Yes. Tagging...

@miss-islington

Copy link
Copy Markdown
Contributor

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

@miss-islington

Copy link
Copy Markdown
Contributor

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

@bedevere-bot

Copy link
Copy Markdown

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

@bedevere-bot

Copy link
Copy Markdown

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 1, 2021
…ythonGH-23127)

This reverts commit aca67da.
(cherry picked from commit b5711c9)

Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 1, 2021
…ythonGH-23127)

This reverts commit aca67da.
(cherry picked from commit b5711c9)

Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
@miss-islington

Copy link
Copy Markdown
Contributor

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

@miss-islington

Copy link
Copy Markdown
Contributor

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

@bedevere-bot

Copy link
Copy Markdown

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 4, 2021
…ythonGH-23127)

This reverts commit aca67da.
(cherry picked from commit b5711c9)

Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
@bedevere-bot

Copy link
Copy Markdown

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 4, 2021
…ythonGH-23127)

This reverts commit aca67da.
(cherry picked from commit b5711c9)

Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
miss-islington added a commit that referenced this pull request Mar 4, 2021
…uest (GH-23127) (GH-24750)

This reverts commit aca67da.
(cherry picked from commit b5711c9)


Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>

Automerge-Triggered-By: GH:jaraco
miss-islington added a commit that referenced this pull request Mar 4, 2021
…uest (GH-23127) (GH-24749)

This reverts commit aca67da.
(cherry picked from commit b5711c9)


Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>

Automerge-Triggered-By: GH:jaraco
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.