synchronized blocks in a Thread class

[drkestrel]

I have a class that looks like the following

public class NetworkingThread implements Runnable
{
    Thread listener;
    Boolean listen;    

    public NetworkingThread()
    {
        listen  = new Boolean(false);
        listener = new Thread(this);
        listener.start()
    }


    public void enableListener()
    {
        listen = new Boolean(true);
    }

    public void disableListener()
    {
        listen = new Boolean(false);
    }
 
    public void run()
    {
        while (true)
        {
            while(listen.booleanValue())
            {
                //listen and process messages
            }
    }
   
}

Note the user create a new NetworkingThread and then invokes the enableListener and disableListener as they wish.

Problem is after the disableListener is first called, the first message that arrived afterwards is processed. Should the following code be sufficient. Isn't the "run" and the "disableListener" meant to be running on the same thread anyway?


public class NetworkingThread implements Runnable
{
Thread listener;
Boolean listen;

public NetworkingThread()
{
listen = new Boolean(false);
listener = new Thread(this);
listener.start()
}


public void enableListener()
{
synchronized(listen)
{
listen = new Boolean(true);
}
}

public void disableListener()
{
synchronized(listen)
{
listen = new Boolean(false);
}
}

public void run()
{
synchronized(listen)
{
while (true)
{
while(listen.booleanValue())
{
//listen and process messages
}
}
}

}
[/code]

 

[idarke]

I think the problem lies in the fact that you're calling the start() method in the constructor. Since this class is already running in the main thread, calling start() in the constructor of this class would start a second instance of NetworkingThread.

Try removing the start call from the class itself and putting it in the parent class that created it:

Constructor is just this:
public NetworkingThread()
{
listen = new Boolean(false);
}

and in your main (or parent) class:

Thread listener = new Thread( new NetworkingThread() );
listener.start();

 

[meadandale]

There are several issues you should address.

First, it is overly expensive with no justification to use a Boolean wrapper instead of a boolean primitive.

Secondly, you synchronize on the flag object but then reassign the reference inside the synchronized block.

You CTOR is fine although it doesn't give the user the fine grained control over starting the thread as might be desired. Perhaps you can overload the CTOR.

I would do things this way:


public class NetworkingThread implements Runnable
{

boolean running = false;

public NetworkingThread(boolean run)
{
if (run)
new Thread(this).start();
}

public NetworkingThread()
{

}


public synchronized void enableListener()
{
running = true;
}

public synchronized void disableListener()
{
running = false;
}

public synchronized boolean isRunning()
{
return running;
}

public void run()
{

enableListener();

while(isRunning())
{
//listen and process messages
}
}


}

alternately, you can change the synchronized methods to:

public void enableListener()
{
synchronized(this)
{
running = true;
}
}

but in essence it is the same thing.

 

[drkestrel]

meadandale- Thanks
The reason I use a Boolean object is I want to synchronized access to the boolean. I started that off because I noticed that the first message after disableListener() is processed in the run method. Would I not need this? I copy the reference in the run method cos I don't want it to hold the lock forever so the caller(parent) can't call disableListener?? Is this right?? Or would I be better of doing Thread.yield or some wait/notifyAll()??

 

[meadandale]

It is more usual to do a synchronize on 'this' or a semaphore object that is used strictly for that purpose.

This should solve your problem. You don't need to do any complicated yielding or notification for a simple runnable like this. If you want to do a producer/consumer then you would.