Bad karma in stopThread

General JUCE discussion
1

I have recently been experiencing bad karma when stopping my threads and I cannot figure out what I am doing wrong. I have not had these issues from before, so it might be related to upgrading to Juce 3.2.0, although it has worked fine for a month or so with the new version. The issue only arises now and then, however, so I might just have been lucky so far.

I run into the bad karma jassertfalse here (juce_Thread.cpp):

bool Thread::stopThread (const int timeOutMilliseconds)
{
    // agh! You can't stop the thread that's calling this method! How on earth
    // would that work??
    jassert (getCurrentThreadId() != getThreadId());

    const ScopedLock sl (startStopLock);

    if (isThreadRunning())
    {
        signalThreadShouldExit();
        notify();

        if (timeOutMilliseconds != 0)
            waitForThreadToExit (timeOutMilliseconds);

        if (isThreadRunning())
        {
            // very bad karma if this point is reached, as there are bound to be
            // locks and events left in silly states when a thread is killed by force..
            jassertfalse;
            Logger::writeToLog ("!! killing thread by force !!");

            killThread();

            threadHandle = nullptr;
            threadId = 0;
            return false;
        }
    }

    return true;
}

NetworkExplorer.h

class NetworkExplorer : public juce::Thread
{
// Other non-related stuff here

private:
    juce::ScopedPointer<DatagramSocket> mSocket;
    boolean mExploring;
}

NetworkExplorer.cpp

NetworkExplorer::NetworkExplorer(bool enableBroadcasting = false) : juce::Thread("NetworkExplorer"),
{
    // The below used to be
    // mSocket = new DatagramSocket(55555, enableBroadcasting)
    // before updating Juce version.

    mSocket = new DatagramSocket(enableBroadcasting);
    mSocket->bindToPort(55555);
}

NetworkExplorer::~NetworkExplorer()
{
    // Used to have
    // mSocket->close();
    // here before updating Juce version.

    mExploring = false;
    this->stopThread(1000);
}

// Other non-related functions, including NetworkExplorer::run().

void NetworkExplorer::startExploring()
{
    mExploring = true;
    this->startThread();
}

Now, the instance of the NetworkExplorer class lives in another class as ScopedPointer:

private:
    ScopedPointer<NetworkExplorer> mNetworkExplorer;

It is created like this in the owner class constructor:

mNetworkExplorer = juce::ScopedPointer<NetworkExplorer>(new NetworkExplorer(false));
mNetworkExplorer->startExploring();

And destroyed in the owner class destructor:

// This is where the code digs into Thread::stopThread() and bad karma is applied.
mNetworkExplorer = nullptr;

Is this not the proper way to create and destroy a custom Thread? What am I doing wrong?

2

Well, your run() method has to keep checking whether the thread is being killed and return if that's the case. If it ignores that and carries on running then when the timeout is reached, of course there'll be problems.

3

Yes, it seems I left out too much of my code for simplicity. I updated the above with a boolean member mExploring that is supposed to handle this, but maybe it is not so? The NetworkExplorer::run() looks like this:

void NetworkExplorer::run()
{
    while (mExploring)
    {
        socketReadyForReading = mSocket->waitUntilReady(true, 1000);
        if (socketReadyForReading == -1)
            continue;

        do
        {
            int numberOfBytesRead = mSocket->read(buffer, 1024, false);
            if (!mExploring)
            {
                return;
            }
        } while (numberOfBytesRead == -1);

        juce::String currentBuffer = juce::String(juce::CharPointer_UTF8(buffer));
        // Do stuff to currentBuffer.
    }
}
4

You should read the docs for Thread::run()

5

Yes I should (and now I have). I have moved the call to stopThread() away from the destructor and call stopThread() in the owner class instead of setting it to a nullptr.

After doing some digging, the main problem seems to be something else, however. When calling DatagramSocket::read() with shouldBlock = false inside NetworkExplorer::run it further calls SocketHelpers::readSocket() and then

bytesThisTime = ::recv (handle, buffer, numToRead, 0);

inside the loop that reads from the socket. At this point the thread is blocked. It just stands there, waiting for new broadcasts to read. If no broadcasts are found it blocks forever. This means that if there is no broadcasts to consume at the point the Thread is to be stopped it will fail because the Thread is currently blocked.

Is this the expected behavior? Am I getting this wrong?

I am working on Windows 8 at the moment.

 

6

Well, calling a function that blocks in your thread will of course prevent it from terminating. You could add a timeout to your socket reader, or maybe send a dummy byte to the socket when you've told your thread to stop. Up to you to do what's appropriate really, depending on your app.

7

Ok, cool, think I get it now. Still wrapping my mind around the network stuff, though. Does DatagramSocket::waitUntilReady() guarantee that DatagramSocket::read() will be successful (i.e. non-blocking) if it returns 1? It seems to work as expected as long as the waiting time passed to stopThread() is large enough (above 1000 in this case). It does not matter if my application takes some seconds to close, it just runs some automated stuff.

void NetworkExplorer::run()
{
    char    buffer[BUFFER_SIZE];
    int     numberOfBytesRead;
    int     socketReadyForReading = -1;

    while (!threadShouldExit())
    {
        socketReadyForReading = mSocket->waitUntilReady(true, 1000);
        if (socketReadyForReading <= 0)
            continue;

        int numberOfBytesRead = mSocket->read(buffer, 1024, false);

        // Do stuff to the buffer.
    }
}
8

Hmm... shouldnt you rather do

mNetworkExplorer = new NetworkExplorer(false);

than

mNetworkExplorer = juce::ScopedPointer<NetworkExplorer>(new NetworkExplorer(false));

 

9

Well, both ways work. The way I have done it at the moment calls the this constructor

/** Creates a ScopedPointer that owns the specified object. */
inline ScopedPointer (ObjectType* const objectToTakePossessionOf) noexcept
    : object (objectToTakePossessionOf)
{
}

while your way uses the = operator

/** Changes this ScopedPointer to point to a new object.
    If this ScopedPointer already points to an object, that object
    will first be deleted.
    The pointer that you pass in may be a nullptr.
*/
ScopedPointer& operator= (ObjectType* const newObjectToTakePossessionOf)
{
    if (object != newObjectToTakePossessionOf)
    {
        ObjectType* const oldObject = object;
        object = newObjectToTakePossessionOf;
        ContainerDeletePolicy<ObjectType>::destroy (oldObject);
    }
    return *this;
}

I guess your way is more explicit and readable though, so it should probably be preferred over my implicit construction. yes