Do not kill deletable tasklets in PyThreadState_Clear

[ghost]

Originally reported by: RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew)

---

(originally reported in Trac by @akruis on 2013-06-03 11:49:35)

Problem

If you run a tasklet only partially (the tasklet calls stackless.schedule_remove() ) and then pickle the tasklet for later execution, you do not want Python to kill the tasklet during PyThreadState_Clear.

The following test code demonstrates the problem:

import stackless
import threading

finally_run_count=0

def task():
    global finally_run_count
    try:
        stackless.schedule_remove(None)
    finally:
        finally_run_count += 1

def run():
    global tasklet
    t = stackless.tasklet(task)
    t()
    stackless.run()
    
    # if you comment the next line,
    # then t will be garbage collected 
    # at the end of this function. 
    tasklet = t

thread = threading.Thread(target=run)
thread.start()
thread.join()
tasklet = None

print "finally_run_count: %d" % (finally_run_count,)

This script emits "finally_run_count: 1". If you comment the line tasklet = t Python deletes the tasklet t during the execution of the thread. As a consequence the finally clause of function task won't be called and the script would print "finally_run_count: 0". Otherwise, the tasklet still exists when Python clears the thread state. In this case PyThreadState_Clear calls slp_kill_tasks_with_stacks which kills the tasklet.

Unfortunately it is not always easily possible to delete the tasklet in time. For instance an object that is part of a not yet collected reference cycle can reference the tasklet and delay its destruction. (This happened in our application flowGuide. Depending on the execution of the garbage collector a tasklet was killed or simply deleted.)

I propose to change the C-function slp_kill_tasks_with_stacks() to not kill a tasklet, if both conditions are met:

• The tasklet can be deleted (C-function tasklet_has_c_stack() returns 0.
• The tasklet is not scheduled (one of stackless.schedule_remove() or tasklet.remove() has been called).

Does this change make sense? Are there any pitfalls?

---

• Bitbucket: https://bitbucket.org/stackless-dev/stackless/issue/24

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@akruis on 2013-09-05 10:45:53 said:

Replying to [comment:10 akruis]:

The current patch is at https://bitbucket.org/akruis/fg2python/compare/2.7-slp-ticket24..2.7-slp#diff

Unless somebody raises an objection, I'll commit the patch to 2.7-slp within the next few days.

I added the latest version of the state chart to the documentation of class tasklet. I'm going to merge
https://bitbucket.org/akruis/fg2python/compare/2.7-slp-ticket24..2.7-slp#diff into 2.7-slp.

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@krisvale on 2013-06-03 20:49:46 said:

I see.
Garbage collection already tries to be clever about seeing whether the tasklet needs to be killed or not, and this is why you see it just stopping its existence like this.
We could, do the same when clearing the thread state, as you suggest.

However, this is really a point where we need to start considering semantics. Consider, for example, if you disable soft-switching. In that case, your comment should have no effect.

also, what is the meaning of try/finally? Is it acceptable to write code that skips the finally clause? I am inclined to think that we should go in the opposite direction, i.e. always guarantee that a tasklet exits, rather than to support their possible abrupt termination, that is then dependent on some random internal factors, such as the existence of a c state or not.

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@akruis on 2013-09-05 13:08:16 said:

I merged the change into 2.7-slp. Changeset [2612a56e0082]

To do: eventually port the change to 3.x-slp

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@akruis on 2013-06-04 10:10:20 said:

Many thanks for your comment.

Replying to [comment:1 krisvale]:

I see.
Garbage collection already tries to be clever about seeing whether the tasklet needs to be killed or not, and this is why you see it just stopping its existence like this.
We could, do the same when clearing the thread state, as you suggest.

However, this is really a point where we need to start considering semantics. Consider, for example, if you disable soft-switching. In that case, your comment should have no effect.

That is completely correct. My concern is actually semantics. But it depends on the point of view: traditionally every Python program is executed by a single instance of the Python interpreter (neglecting multiprocessing and the like). With Stackless Python I'm able to execute certain Python programs using several interpreter instances: the program runs as a tasklet, I pickle the tasklet and continue it later. The developer of such a program expects a finally block to be executed exactly once.

About soft switching: all of this depends on soft switching and does not work with hard switching. (If there was a switch to disable hard switching, I could turn it off. My usage of Stackless is different from most other applications, because I do not need lightweight threads or channels at all. I just need the ability to serialise tasklets.)

also, what is the meaning of try/finally? Is it acceptable to write code that skips the finally clause?
I think it is acceptable. os.exit() is also acceptable.

I am inclined to think that we should go in the opposite direction, i.e. always guarantee that a tasklet exits, rather than to support their possible abrupt termination, that is then dependent on some random internal factors, such as the existence of a c state or not.

Here I disagree partially. If you think of a tasklet as a kind of coroutine in a traditional program (a program that could be written using greenlets), your completely right. But if you extend the concept of classical programs and add pickling and unpickling of tasklets, we have a new situation. The "existence of a c state" is no longer a random internal factor. It is crucial to avoid c state. If your are able to create "alive" tasklets via unpickling you definitely need the opposite operation too.

But this discussion definitely belongs to the mailing list, not to this ticket.

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@ctismer on 2013-09-05 15:05:30 said:

Great, thanks for the nice chart!

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@akruis on 2013-06-10 17:50:03 said:

After some days of pondering I don't like my initial proposal to handle some tasklets in slp_kill_tasks_with_stacks() in a special way any longer. The conditions are simply to arcane.

A few days ago I started a discussion on the mailing list (http://www.stackless.com/pipermail/stackless/2013-June/005704.html), but nobody responded. In this message, I mentioned a few alternative ideas to resolve this issue. Here is another one. This time I also prepared a patch.

Let's look at the life cycle of a tasklet

                      State: tasklet does not exist
                                 ↓       ↑
            stackless.tasklet()  ↓       ↑   tasklet_dealloc(PyTaskletObject *)
                                 ↓       ↑
                      State: tasklet is not alive  ← ← ← ← ← ← ← ← ←
                                 ↓       x                          ↑
tasklet.bind(); tasklet.setup()  ↓       x   MISSING                ↑
                                 ↓       x                          ↑
                      State: tasklet is alive                       ↑ run to end
                                 ↓       ↑                          ↑     or
               tasklet.insert()  ↓       ↑   tasklet.remove()       ↑ tasklet.kill()
                                 ↓       ↑                          ↑
                      State: tasklet is scheduled                   ↑
                                 ↓       ↑                          ↑
                stackless.run()  ↓       ↑   stackless.schedule()   ↑
                                 ↓       ↑                          ↑
                      State: tasklet is running  → → → → → → → → → →

Actually this diagram is not quite correct, but you can get the point. There is no inverse operation for tasklet.bind(); tasklet.setup(). In the diagram this operation is marked as "MISSING". I propose to use tasklet.bind(None) as MISSING. This requires two changes to tasklet.bind():

• Allow rebinding of a tasklet, that is alive, not scheduled and restorable. In this case tasklet.bind() drops frames and cstate.
• Accept None as argument value.

The attached patch implements this change, adds test cases and updates the documentation.

The change to PyTasklet_Bind(PyTaskletObject *task, PyObject *func) needs to be carefully reviewed. It is probably correct, but the code to the cstate handling has been copied from bind_tasklet_to_frame(). We could factor this code into a static function.

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@krisvale on 2013-06-04 10:48:57 said:

I agree. I also can see your point. Would you care to pose it? The pickling and resuming case is compelling. I wonder if we should mark a tasklet as being "alive" in the sense that it is pickled somewhere....

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@krisvale on 2013-06-11 09:13:25 said:

Aha, this is interesting. Excplicitly unbinding the tasklet.
I admit that I had forgotten about the bind/setup functions. I agree, imho it is probably better to do this explicitly rather than to create yet another sort of global rule about what happens to tasklets...

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@akruis on 2013-06-11 13:45:30 said:

Replying to [comment:6 krisvale]:

The patch looks ok, but I agree with your comment that we shouldn't try to do state manipulation when unbinding the tasklet.
Are you sure, that there are no side effects of deleting frames and not changing cstate? I do not overlook the code well enough.

Replying to [comment:7 ctismer]:

Very interesting topic, actually.

I would like to compare the two patches.
Do you have them in another repo?

I'm sorry, there is only one patch. I didn't implement my initial proposal. The current patch is at
https://bitbucket.org/akruis/fg2python/commits/83cc2cecb7896e1a2a88a41b55f36e3a5b8f8bce?at=2.7-slp-ticket24

Or can you maybe make a branch, apply both patches and check that in,
so that the versions are visible and comparable?
We can then think a bit and finally merge it into trunk.

Also the state diagram is nice to keep in the source as a comment.

We could add it to the documentation, because it covers tasklets in general.

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@krisvale on 2013-06-11 09:17:11 said:

The patch looks ok, but I agree with your comment that we shouldn't try to do state manipulation when unbinding the tasklet.

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@ctismer on 2013-06-11 12:34:37 said:

Very interesting topic, actually.

I would like to compare the two patches.
Do you have them in another repo?

Or can you maybe make a branch, apply both patches and check that in,
so that the versions are visible and comparable?
We can then think a bit and finally merge it into trunk.

Also the state diagram is nice to keep in the source as a comment.

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@krisvale on 2013-06-12 09:32:10 said:

Replying to [comment:8 akruis]:

Are you sure, that there are no side effects of deleting frames and not changing cstate? I do not overlook the code well enough.

Pretty sure. The cstate is only used if someone tries to activate the tasklet and this should not happen unless it is bound. An unbound tasklet should probably have no cstate associated with it.

This cstate business is a bit weird though and we could refactor that in a separate step, if you like. I'm also working on a separate project to perform stack management using tealets and that will probably affect the cstate stuff as well.

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@akruis on 2013-09-04 12:56:23 said:

Replying to [comment:7 ctismer]:

Also the state diagram is nice to keep in the source as a comment.

The original ASCII art state diagram is not quite correct. But a state diagram could be indeed helpful. I tried to model it in Enterprise Architect. Here is a first draft. We could integrate it into the documentation similar to the logging flow diagram in the Python Logging Howto http://docs.python.org/2/howto/logging.html#logging-flow

'''Improved version 3'''

[[Image(https://bitbucket.org/akruis/fg2python/downloads/tasklet_state_chart_v3.png)]]

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@akruis on 2013-06-18 07:57:22 said:

Replying to [comment:9 krisvale]:

Pretty sure. The cstate is only used if someone tries to activate the tasklet and this should not happen unless it is bound. An unbound tasklet should probably have no cstate associated with it.

Fine. I removed the cstate manipulation from PyTasklet_Bind().

The current patch is at https://bitbucket.org/akruis/fg2python/compare/2.7-slp-ticket24..2.7-slp#diff

Unless somebody raises an objection, I'll commit the patch to 2.7-slp within the next few days.

[ghost]

Original comment by Anselm Kruis (Bitbucket: akruis, GitHub: akruis):

---

Resolved since commit 2612a56e0082.