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

new method tasklet.bind_thread() #26

Closed
ghost opened this issue Nov 29, 2013 · 20 comments
Closed

new method tasklet.bind_thread() #26

ghost opened this issue Nov 29, 2013 · 20 comments

Comments

@ghost
Copy link

ghost commented Nov 29, 2013

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


(originally reported in Trac by @akruis on 2013-11-28 10:46:33)

Thanks to Kristján Stackless Python got a new method: tasklet.bind_thread(). I won't discuss the motivation and the implementation details of this method here. It's all in this thread of the Stackless mailing list:
http://www.stackless.com/pipermail/stackless/2013-November/005869.html

The purpose of this ticket is to add all the missing details we need for the next release (2.7.6) of Stackless.

  • document the method in Doc/library/stackless/tasklets.rst
  • update the documentation in Doc/library/stackless/threads.rst to clearly describe the relation between
    a tasklet and a thread:
    • A tasklet always has an associated thread. This thread is identified by the property thread_id
    • The thread_id of a tasklet changes only if the method bind_thread() is called or if the associated thread terminates.
      In the later case the new thread is ... \
      @KristJán here I need your input: I noticed, that the thread id changes to the main thread,
      but what happens if the main thread terminates while other threads are still active?
  • update Stackless/changelog.txt

I'll take care about these points myself, but I need help from Kristján to add the missing information.


@ghost
Copy link
Author

ghost commented Nov 29, 2013

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


@ctismer on 2013-11-28 22:34:56 said:

Aside:

If we are moving the issue tracker to BB, is there a way to import the old issues of the stackless
site, or any update/sync possible in any way?

I would not like to loose the existing history, really.

I do like the easy administration of BB.
At the same time, Stackless is more complicated, but it is still superior since we have the full control,
if we understand it (I don't either).

Do you see a way out of this dilemma?

Cheers - chris

@ghost
Copy link
Author

ghost commented Nov 29, 2013

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


@krisvale on 2013-11-28 12:34:41 said:

The reason is that when a thread dies, its PyThreadState goes away.
Each tasklet points to a PyCStackObject which again points to a thread state
tasklet->cstate->tstate

The thread ID is taken from the tstate.
If a thread goes away, then at the very least we should not point to its non-existent thread state any more. (The reason that it used to work is that the killing of tasklets was broken. Dangling pointers were used to get that information. The current (broken and incomplete, I see now) code assigns such tasklets to the main thread, by modifying the tasklet->cstate pointer.

Now, I see that some of the logic here is still broken. No one can point to a thread state after the thread exits, and I do keep some of those pointers hanging around. Also, there is a reference leak that I need to fix. So, I still need to work on this.

The simple fix is to re-assign such tasklets to the main thread, (an implicit re-bind). Is it important to keep the thread_id? Remember that thread-ids are not good identifiers. They may get re-used by new threads

What behaviour would you like to see?

We could put in the feature to have cstate->tstate have a NULl value as an allowed value. This would then indicate that it belongs to a thread that is
no longer existing. It could have repercussions throughout the code, though. Does this sound like a reasonable solution?

@ghost
Copy link
Author

ghost commented Dec 2, 2013

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


The simple fix is to re-assign such tasklets to the main thread, (an implicit re-bind). Is
it important to keep the thread_id? Remember that thread-ids are not good
identifiers. They may get re-used by new threads

What behaviour would you like to see?

Changing the thread id is perfectly valid if you rebind the tasklet to the main thread. I just want to document the user visible part of the behaviour.

So my question is still unanswered: which thread do you bind a tasklet to, if the main thread does no longer exist, but other non daemon threads are still alive?

@ghost
Copy link
Author

ghost commented Dec 2, 2013

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


I have a change in the making where the tasklet is unbound from a thread if the thread is dying and remains unbound. In this case, its thread-id will report as 0. Various scheduling APIs will result in an error. if it can be rebound, i.e. is soft-switchable, then t.bind_thread() can be called.
Just to re-iterate: thread ids are not separately assigned to tasklets, they are a property of the actual thread that the tasklet is bound to.

Now, how does this here work? We don't have forks or pull requests here yet because stackless is not a public repo. Should I check it in, and refer to this ticket?

@ghost
Copy link
Author

ghost commented Dec 4, 2013

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


I'll submit my fix. I will also make changes so that bind_thread() can take a thread_id, and the special thread_id -1 means no-thread.
I'll also make draft changes to stackless.rst

@ghost
Copy link
Author

ghost commented Dec 4, 2013

Original comment by Anonymous:


I will merge the 2.7.6 changes into 2.8.0-rc(x)

How should I set (x) - simply merge and keep rc1, or should I bump that?

@ghost
Copy link
Author

ghost commented Dec 4, 2013

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


Okay, the fix is in along with unittests.
First, a clarification: When a thread dies, only tasklets with a C state are actively killed. soft-switched tasklets simply stop.
All tasklets bound to a thread will have their cstate nerfed, which means that their thread_id will report as "-1". This also include soft-switched tasklets, which share a cstate.

The reason we cannot kill soft-switched tasklets is that there is no central list of them. we only know about hard-switched ones because we maintain a linked list of cstates.

Threads that end really should make sure that they finish whatever worker tasklets they have going by manually killing them, but that is up to application code. The reason we kill the ones with C state is that not doing so can cause serious leaks when a C state is not unwound.

bind_thread now can take an integer thread-id, which can be the thread_id of a stackless thread.

@ghost
Copy link
Author

ghost commented Dec 4, 2013

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


Anselm, can you take care of updating the docs?

@ghost
Copy link
Author

ghost commented Dec 5, 2013

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


I'll update the documentation. Many thanks for the implementation.

Am 04.12.2013 23:08, schrieb Kristján Valur Jónsson:

@ghost
Copy link
Author

ghost commented Dec 5, 2013

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


I just recompiled s.7-slp and run the stackless unittests.
D:\kruis_F\fg2\stackless\stackless\PCbuild>python.exe ..\Stackless\unittests\runAll.py

I got the following error in the second pass:

======================================================================
ERROR: test_bind_to_other_tid (test_thread.SchedulingBindThreadTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\kruis_F\fg2\stackless\stackless\Stackless\unittests\test_thread.py", line 427, in test_bind_to_other_tid
    t.bind_thread(otherThread.ident)
RuntimeError: tasklet has C state on its stack

----------------------------------------------------------------------
Ran 221 tests in 0.360s

FAILED (errors=1, skipped=2)

Hg sum output:

parent: 87559:2db601ba6e79 tip
 Allow bind_thread to take an argument, the thread_id of the target thread.
branch: 2.7-slp
commit: (clean)
update: (current)
mq:     (empty queue)

@ghost
Copy link
Author

ghost commented Dec 6, 2013

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


Actually we are almost done with this issue. The promised documentation update is in commit eb66402fdcd0.

Probably we still need to merge the changes to 2.8-slp and 3.x.

@ghost
Copy link
Author

ghost commented Dec 25, 2013

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


I have not merged anything into 2.8. What is the state of 2.8?

@ghost
Copy link
Author

ghost commented Dec 25, 2013

Original comment by Anonymous:


It is unchanged since a few weeks because of my moving.
And I'm still not quite sure of a few things:

"""I will merge the 2.7.6 changes into 2.8.0-rc(x)
How should I set (x) - simply merge and keep rc1, or should I bump that?
"""

Also, I need a bit advice:
Mentioned elswhere

  • I use the updated 2.7.x and merge the 2.8,
  • rename the executable to stackless and the dll to stackless-something.dll
  • add a new python that uses stackless-something.dll but
    • disables initialisation of stacklessmodule
    • reports itself as "2.7.x+"

Is that valid, still?
Should I continue on the 2.8 branch or do it with pull requests?

@ghost
Copy link
Author

ghost commented Dec 26, 2013

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


I think that for the name of executable/dll, we also need to keep in mind the generation of installables, i.e. the msi.py in tools/msi.

I have also stated this elsewhere, but I am confused as to why we need this "dual" mode for the 2.8 branch. Either be stackless.28 or python 2.7.x (where x is likely to be 99). Dual mode makes everything more complicated.

What is the use case for the latter? If it is only to get VS2010 support for 2.7, then it would be easier to just create a custom branch of cpython 2.7....

I also think that this is not the best place to discuss this :). Issues don't allow threaded discussions (but commits do :) Email, maybe?

@ghost
Copy link
Author

ghost commented Dec 27, 2013

Original comment by Anonymous:


It is simple to change dynamically by inspecting the executable name.
A simple feature to be added, even without adding extra files.
I don't want to loose the chance to switch the same install from stackless
to python, by a simple copy/rename.

@ghost
Copy link
Author

ghost commented Dec 27, 2013

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


But python.exe is not the only executable using python28.dll or whatever the dll will be name. So if you are talking about run-time selection (as opposed to compile time) then we need to be able to do that explicitly.

Also, I still don't get the point of "switching off" stackless. If a customer wants python built for VS 2010, why not just give him python plus the stackless features? They won't get in the way of regular python usage. IMHO we should be using every opportunity to promote Stackless, by giving it to people that don't know that they want it :)

@ghost
Copy link
Author

ghost commented Dec 27, 2013

Original comment by Anonymous:


That exactly is my point - I'm just trying to go both ways.
In my case, the customer wants the best possible python 2.x but is afraid
of stackless. For that reason I want to rename it simply to python 2.7.99
and switch stackless off. (maybe even that is not necessary, I agree).

My trick is to give them the best python, let it install with everything, and
later tell them that they have stackless as well. In the python case, it needs
to report itself as not being piton 2.8.

I think we are in the same direction. In this case, I think of some installation
script that I supply for the customer. It installs stackless and renames to python,
afterwards. They use it and are happy, and I tell them later they may drop the renaming,
and they learn that they had stackless, all the time. Something like that.
It is just a political thing to convince them first that they have a great python,
let them use it, and tell them then that they used stackless all the time already.
It is bad if I make that obvious in the first place...

Maybe even this trickery is not necessary, but I don't want to loose this great
chance by negotiating with them.

@ghost
Copy link
Author

ghost commented Dec 27, 2013

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


Ok, I see your point. Politics are a delicate business and I am not very good at it :) I hope this goes well.

I'm just trying to make sure that we don't get stuck with some sort of a cludge. In particular relying on the executable name rings warning bells with me. There are all sorts of potential unexpected issues. symlinks, embedded systems, etc. But maybe it is okay as a temporary measure :)

In particular, I'm happy that we seem to be abandoning the idea to have conditional compilation to turn features on/off. That would have been horrible to maintain, and real case for two different branches.

@ghost
Copy link
Author

ghost commented Jan 3, 2014

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


This has been merged into 3.x. merging into 2.8 is on its own cadence. Marking this as resolved.

@ghost
Copy link
Author

ghost commented Nov 6, 2016

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


Removing milestone: 2.7.6-slp (automated comment)

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

0 participants