D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 5157 - thread_joinall() looks like it doesn't actually join all
Summary: thread_joinall() looks like it doesn't actually join all
Status: RESOLVED INVALID
Alias: None
Product: D
Classification: Unclassified
Component: druntime (show other issues)
Version: D2
Hardware: Other Linux
: P2 normal
Assignee: Sean Kelly
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-02 20:46 UTC by Jonathan M Davis
Modified: 2015-06-09 05:15 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Jonathan M Davis 2010-11-02 20:46:51 UTC
In core.threads, there is a function called thread_joinall() which supposedly joins all non-daemon threads. At present, it's implementation is

extern (C) void thread_joinAll()
{

    while( true )
    {
        Thread nonDaemon = null;

        foreach( t; Thread )
        {
            if( !t.isDaemon )
            {
                nonDaemon = t;
                break;
            }
        }
        if( nonDaemon is null )
            return;
        nonDaemon.join();
    }
}


Notice that the last if statement _returns_ if the thread is a daemon thread. That means that the function joins every thread until it finds a single daemon thread, so it's _not_ going to join all non-daemon threads unless there are no daemon threads or all of the daemon threads are at the end of the range generated by opApply(). So, if I understand correctly what the function is supposed to be doing, it should be something more like this:


extern (C) void thread_joinAll()
{
    foreach( t; Thread )
    {
        if( !t.isDaemon )
            t.join();
    }
}


This still might have issues though if the list of running threads changes while thread_joinAll() is running. I think that opApply() will iterate over the list of threads as they exist when opApply() is called, but if any threads are joined while opApply() is running (likely because joining a thread causes another thread to join in addition to the one join() was called on), then you could get a double-join happening, which would currently result in an exception being thrown. So, there may also need to be a way to verify that a thread hasn't been joined before attempting to join it. And of course, some locking primitives may be necessary to make sure that there aren't any race conditions in doing so.
Comment 1 Steven Schveighoffer 2010-11-03 07:55:06 UTC
The original code looks correct to me.  Rewritten in pseudocode:

while(true)
{
   nonDaemon = null;
   assign nonDaemon to first non-daemon thread in the list of threads;
   if(nonDaemon not found)
     // this means there are no non-daemon threads left
     return;
   nonDaemon.join();
}

The reason it is done this way instead of the way you suggest is because the foreach is synchronized on a lock (to prevent threads coming/going while iterating).  So you don't want to call join while inside the foreach loop.
Comment 2 Jonathan M Davis 2010-11-03 10:24:45 UTC
You're right. I guess that I was just looking at it too closely or something. But on another inspection, it does seem to be correct. It will loop through all of them until it finds a non-daemon thread or it loops through all of them, and only then will it end up returning. So, this bug isn't a bug.