greenlets leak thread local data when used with threads

[mitsuhiko]

If greenlets are used with threads they will leak weak references to _thread._localdummy objects. These objects seem to be internally used in the interpreter to implement thread local storage.

Simple test case:

import threading
import greenlet

while 1:
    t = threading.Thread(target=greenlet.getcurrent)
    t.start()
    t.join()

I can reproduce this on OS X 32/64 bit in the latest 2.7 version as well as linux 32 bit, both running the 0.3.4 version of greenlet.

[snaury]

Yes, I can confirm that. The leak is not because of _thead._localdummy, however, but because main greenlets themselves are leaking. The problem is that reference to current greenlet is kept in a global variable, and it's only switched when current greenlet is needed in another thread. The problem is that when greenlet is deallocated, it realizes that it's on another thread already, so it schedules itself for deallocation on its own thread, but that thread is gone and will never come back, that's why it's leaking. I'm not sure how to fix this, the problem is there's no reliable way to detect when thread goes away, except using weak references all over the place.

[mitsuhiko]

The problem is that when greenlet is deallocated, it realizes that it's on another thread already, so it schedules itself for deallocation on its own thread

It actually never even tries to deallocate itself. I changed the gc callback to force collection if the thread is dead, but it never results in dealloc being called:

static int green_is_gc(PyGreenlet* self)
{
    /* The owning thread died, return */
    if (PyThreadState_GET()->dict == NULL)
        return 1;
    /* Main and alive greenlets are not garbage collectable */
    return !(self->stack_stop == (char *)-1 || self->stack_start != NULL);
}

[snaury]

Oh, and here's the twist, I cannot create weak references to thread state dict:

TypeError: cannot create weak reference to 'dict' object

[snaury]

It actually never even tries to deallocate itself. I changed the gc callback to force collection if the thread is dead, but it never results in dealloc being called:

The path should be something like this: green_dealloc -> check active -> kill_greenlet -> thread mismatch -> add to ts_delkey. But I'll double check, of course.

[mitsuhiko]

Isn't the problem that green_dealloc is not called in the first place? No matter what I do, i can't get the GC to actually consider that thing garbage and kick in deallocation.

[snaury]

Oh, you are right, it doesn't even go to green_dealloc, because ts_current is saved in threadstate dict, so there's an early cycle.

[snaury]

The thing is, it was designed that way, because when greenlet is running on some random thread, ts_current might be the only reference to current greenlet. When it comes back to that thread it should be restored. If I could have a callback just before thread dies, this would be as easy as Py_XDECREF(ts_current) with additional checks for ts_current == NULL, but I can't see how to do that.

[mitsuhiko]

Well, you can easily do that by instead of storing the dict, you store the threadstate instead. Then every time you get the dict you check if the dict is null. if the dict is null the thread went away and is waiting for the cycle to cleanup -> decref.

[snaury]

You cannot store thread state, because thread state is not reference counted. After PyThreadState_DeleteCurrent() call in threadmodule.c the pointer would no longer be valid and might point to any arbitrary location. You cannot check anything after that.

[snaury]

Ok, I have a proof of concept patch that fixes your particular sample, but it's a hack and I won't commit it to the main repo. But it might help you privately:

diff --git a/greenlet.c b/greenlet.c
index 68caa4e..2683e89 100644
--- a/greenlet.c
+++ b/greenlet.c
@@ -214,8 +214,11 @@ static int green_updatecurrent(void)

        /* save ts_current as the current greenlet of its own thread */
        previous = ts_current;
-       if (PyDict_SetItem(previous->run_info, ts_curkey, (PyObject*) previous))
-               return -1;
+       /* HACK: only store ts_current if run_info is not abandoned */
+       if (Py_REFCNT(previous->run_info) != 1) {
+               if (PyDict_SetItem(previous->run_info, ts_curkey, (PyObject*) previous))
+                       return -1;
+       }

        /* get ts_current from the active tstate */
        tstate = PyThreadState_GET();
@@ -673,6 +676,11 @@ static int green_init(PyGreenlet *self, PyObject *args, PyObject *kwargs)

 static int kill_greenlet(PyGreenlet* self)
 {
+       if (self->stack_start == (char*)1 && self->stack_stop == (char*)-1) {
+               /* HACK: main greenlet, just abandon! */
+               self->stack_start = NULL;
+               return 0;
+       }
        /* Cannot raise an exception to kill the greenlet if
           it is not running in the same thread! */
        if (self->run_info == PyThreadState_GET()->dict) {

[snaury]

Bear in mind that it would only work if by the time thread exits there's only main greenlet active in that thread. If you have multiple greenlets, and you switch to main greenlet and cause a thread exit, this hack would no longer work.

[mitsuhiko]

The proper solution would be a helper object that is stored in the tls that on deallocation notifys the greenlet that the thread went away. that helper object would have the run state instead of the greenlet itself. If the reference from the greenlet to the helper object is weak that should work (idea by Antoine Pitrou).

[snaury]

Hmm, at first I thought that would work, but then I realized that it wouldn't. That would mean we can't keep references to thread dict, but we need it to detect cross-thread switches, and if helper object keeps a reference to it, then it wouldn't work, because then situation wouldn't change at all, thread dict wouldn't go away, helper object wouldn't be deallocated. Even if it could work, it would be way too complicated, and I speed would likely suffer a lot.

In the mean time, I worked a little more on the hack, it seems I managed to make it a little better (and fixed a glaring bug), so I'm more confident with it. Please see if this helps you with leaks: https://github.com/snaury/greenlet/tree/exp-thread-leaks

If it does, I would commit it to the main repo.

[mitsuhiko]

and if helper object keeps a reference to it

That's why it would keep a reference but would not own it.

[snaury]

That's why it would keep a reference but would not own it.

See above, when I tried, I got:

TypeError: cannot create weak reference to 'dict' object

[mitsuhiko]

On the c level you can represent weak references by just not incrementing the refcount and not decrementing it alter. That of course means you wouldn't be able to figure out if the reference died but that is not a problem in this situation because you know the order of destruction.

[snaury]

Ok, I understand what you mean now. But that's:

1. Way too complicated, if you can come up with a patch, you're welcome
2. Since thread switches can happen at any time, you would likely have to get rid of ts_current, and use thread state for that (so that thread dict is the only place where you keep references to greenlets), thus you'd have to update it on every switch, which is an additional level of failure, unpredictability and performance hit, in an extremely sensitive place (the end of g_switchstack).

After a monstrosity of a rewrite like that, I doubt you would gain much, but you are free to try.

[mitsuhiko]

Since thread switches can happen at any time

If only there was a global interpreter lock :D

That being said, I will try to come up with a patch for that.

[snaury]

If only there was a global interpreter lock :D

What I meant is that some calls from C to Python might execute Python code and/or release the GIL, which might switch threads, so it adds a lot of complexity especially closer to the core of the switching code, where switch arguments are transferred via global variables. Just saying.

[mitsuhiko]

The patch behind the pull request fixes the common issue. Some notes on it:

• tp_is_gc should not switch return values depending on the state of the object. That would confuse the gc
• it's not necessary to prevent the current greenlet or the main one from being collected because there should always be reachable anyways. It's either in ts_current or the tls.

[snaury]

I realized what worries me. The problem is that it might be possible tp_clear would cause greenlet switches, which is a big no and can lead to crashes. What if Py_CLEAR(self->parent) causes parent greenlet to deallocate, although I can't prove it's possible yet, but if parent is alive and kill_greenlet kicks in, then stack switching might corrupt gc's unreachable lists, which are on the stack. This is actually why I made sure that active greenlets would not be garbage collected, because I proved crashes before with a test case in my wip-full-gc branch.

[mitsuhiko]

What if Py_CLEAR(self->parent) causes parent greenlet to deallocate

Then that greenlet will not have a thread either which means that the cleanup will not ever attempt to switch. Since all greenlets will be owned by the same thread the same cleanup logic is applied to all: which is mostly just deallocate and otherwise noop.

[snaury]

You might miss the fact that your lack of tp_is_gc means that it's not just about missing thread anymore. Ordinary greenlets that would not be considered by gc, are considered now, which means active greenlets in cycles might garbage collect now. It's late and I can't figure out how to properly make a cycle from g2 to g1, but when I add tp_dictoffset to greenlet, then this sample shows that there's a switch inside gc, which under right conditions might crash:

import gc
import sys
import weakref
from greenlet import greenlet, GreenletExit

main = greenlet.getcurrent()

def func(seen):
    try:
        main.switch()
    except:
        print sys.exc_info()
        seen.append(sys.exc_info()[0])
        raise
    raise RuntimeError

seen = []
g1 = greenlet(func)
g2 = greenlet(func)
g1.parent = g2
g1.switch(seen)
g2.switch(seen)
g2.ref = g1
print 'g1', g1
print 'g2', g2
del g2
del g1
print 'gc.collect start'
gc.collect()
print 'gc.collect end'
print seen

Output:

g1 <greenlet.greenlet object at 0x10043bc30>
g2 <greenlet.greenlet object at 0x10043beb0>
gc.collect start
green_clear(0x10043bc30)
green_dealloc(0x10043beb0)
(<class 'greenlet.GreenletExit'>, GreenletExit(), <traceback object at 0x1004dde18>)
green_dealloc(0x10043bc30)
gc.collect end
[<class 'greenlet.GreenletExit'>]

[mitsuhiko]

The problem there is that the whole idea of having tp_is_gc return different values on the state of the object is probably not the best idea ever. To avoid clearing the currently active object you could probably just roll that logic into the traverse method?

[snaury]

I think it's ok, as long as there are strong possibilities that is_gc won't change during gc. Changing your mind in traverse is actually worse, because unlike is_gc it's called multiple times, and that would certainly confuse gc.

But anyway, I managed to make your idea work, see https://github.com/snaury/greenlet/tree/wip-next (it's kind of a post-midnight braindump at the moment though, with multiple unrelated patches, @schmir if you're reading this, please look if you don't like any of those). Thanks a lot for your help, using gc certainly makes it a lot cleaner. :)

[snaury]

Oops, I was wrong, is_gc is called more than once. :) But it doesn't change the fact that anyone switching greenlets (which may flip is_gc) during gc is shooting themselves in the foot...

[snaury]

Ouch, it seems my logic is faulty, it's perfectly possible to switch to active greenlets in gc, as easy as this:

import gc
import weakref
from greenlet import greenlet

def alive():
    try:
        greenlet.getcurrent().parent.switch()
    finally:
        print 'Dying!'

g = greenlet(lambda: None)
g.ref = g
g.alive = greenlet(alive)
g.alive.switch()
g = weakref.ref(g)
assert g() is not None
print 'Before collect'
gc.collect()
print 'After collect'
assert g() is None

This prints:

Before collect
Dying!
After collect

Which means that it's possible to switch in gc, which means even current releases are not as safe as I thought they are... :(

[snaury]

sigh Ok, it's not that bad, actually. If I'm thinking straight, then since active greenlets are not collectible, then they won't appear in unreachable nor finalizers lists, neither anything they reference will appear in those. As a result, after the switch, and until that switch returns, no deallocations should touch unreachable/finalizers in gcmodule.c/collect, because, if I'm not mistaken here, greenlet can only ever deallocate what it can reach, and that's by definition reachable from it. So as long as active greenlets are not garbage collectible we're good. ;)

[mitsuhiko]

\o/

[schmir]

I'm sick at the moment and didn't follow most of the discussion in depth. @snaury, feel free to apply your changes as you see fit.

[snaury]

Today I realized that your sample script was still leaking with new code and found the culprit (deallocation order could bypass greenlet's tp_clear, because threadstate->dict might be cleared first). The changes are now in master, so I'm closing this issue.

[mgius]

Looks like no Canonical blessed ubuntu .deb exists with this fix in it. I think the two commits I need to cherry-pick are:

744eef4
00e583d

Is this correct?

[snaury]

@mgius tests for that leak depend on a new feature (per-greenlet __dict__, a4cc1c9), so you'd have to remove dependency on that. Additionally, you'd probably need dae2314.

[mgius]

That's good to know. It might be easier just to take 0.4.0 and package that up instead. Thanks for your reply.