Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Killing greenlets across threads may leak greenlets and their stacks? #252

Closed
jamadden opened this issue Aug 6, 2021 · 3 comments · Fixed by #261
Closed

Killing greenlets across threads may leak greenlets and their stacks? #252

jamadden opened this issue Aug 6, 2021 · 3 comments · Fixed by #261
Assignees

Comments

@jamadden
Copy link
Contributor

jamadden commented Aug 6, 2021

If proper APIs aren't called in the target thread. See #245 (comment)

The analysis there may not actually be correct. There is a reference cycle, but the greenlets contained in the deleteme list should be subject to garbage collection (eventually) because none of them should be active after the thread dies.

@jamadden
Copy link
Contributor Author

jamadden commented Aug 7, 2021

Similarly, there's no cleanup of the main greenlet from a thread unless some other thread calls one of those proper APIs. The main greenlet from a thread is left "dangling" in a suspended state until the user explicitly updates the state from a different thread. For example, this always reports a growth in greenlets:

import gc
import threading

import greenlet

def count_objects(kind=list):
    # pylint:disable=unidiomatic-typecheck
    # Collect the garbage.
    for _ in range(3):
        gc.collect()
    gc.collect()
    return sum(
        1
        for x in gc.get_objects()
        if type(x) is kind
    )

def target():
    print(greenlet.getcurrent())

print("Begin glets", count_objects(greenlet.greenlet))
t = threading.Thread(target=target)
t.start()
t.join()
del t
print("End glets", count_objects(greenlet.greenlet))
$ python foo.py
Begin glets 1
End glets 2

Inserting a call to greenlet.getcurrent() after del t resolves the issue.

@jamadden
Copy link
Contributor Author

jamadden commented Aug 7, 2021

With #251 fixed (when you call getcurrent() in both threads), the list still leaks if you don't call getcurrent() in the background thread before it exits. Both the main and secondary greenlet from the background thread are still reachable by GC, GC is in fact traversing into them both, and yet no cycles are broken (green_clear is never called).

@jamadden jamadden mentioned this issue Aug 7, 2021
@jamadden jamadden self-assigned this Sep 28, 2021
@jamadden
Copy link
Contributor Author

Sigh. I have a branch that partly fixes this (I can solve the list leak), but there are still cases where it's very difficult to fix completely. If you start a greenlet in a background thread, provide it to another thread, don't let the background greenlet complete, and don't have the background thread call a greenlet API before finishing and after the last reference to the background greenlet has been deleted, then at least the main greenlet from the background thread leaks (or more, depending on the parent chain).

See

def test_issue251_killing_cross_thread_leaks_list(self, manually_collect_background=True):
, as called (passing False for the parameter) by
def test_issue251_issue252_need_to_collect_in_background(self):

Currently, the frames from the secondary background greenlet also leak. My branch can at least fix that much. It may not be possible to fix the whole thing though, because a leaking reference is in the C stack and inaccessible. I still have a few things to try, though.

jamadden added a commit that referenced this issue Sep 30, 2021
We're now using Py_AddPendingCall to clean up some thread-local things that don't clean themselves up. I'm still working to be sure we break cycles.
jamadden added a commit that referenced this issue Oct 2, 2021
I haven't tested this on other platforms or with a more complex app like gevent yet though.
jamadden added a commit that referenced this issue Oct 12, 2021
We're now using Py_AddPendingCall to clean up some thread-local things that don't clean themselves up. I'm still working to be sure we break cycles.
jamadden added a commit that referenced this issue Oct 12, 2021
I haven't tested this on other platforms or with a more complex app like gevent yet though.
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 a pull request may close this issue.

1 participant