Where to stop/destroy threads in Android Service class?

Question

I have created a threaded service the following way:

public class TCPClientService extends Service{  
...

@Override
public void onCreate() {
    ...
    Measurements = new LinkedList<String>();
    enableDataSending();    
}

@Override
public IBinder onBind(Intent intent) {
    //TODO: Replace with service binding implementation
    return null;
}

@Override
public void onLowMemory() {
    Measurements.clear();
    super.onLowMemory();
}

@Override
public void onDestroy() {
    Measurements.clear();
    super.onDestroy();
    try {
        SendDataThread.stop();
    } catch(Exception e){
        ...     
    }

}



private Runnable backgrounSendData = new Runnable() {

    public void run() {
        doSendData();
    }
};

private void enableDataSending() {
    SendDataThread = new Thread(null, backgrounSendData, "send_data");
    SendDataThread.start();
}

 private void addMeasurementToQueue() {
     if(Measurements.size() <= 100) {
         String measurement = packData();
         Measurements.add(measurement);
     }
 }

 private void doSendData() {
     while(true) {
         try {      
             if(Measurements.isEmpty()) {
                 Thread.sleep(1000);
                 continue;
             }
             //Log.d("TCP", "C: Connecting...");
             Socket socket = new Socket();
             socket.setTcpNoDelay(true);
             socket.connect(new InetSocketAddress(serverAddress, portNumber), 3000);
             //socket.connect(new InetSocketAddress(serverAddress, portNumber));
             if(!socket.isConnected()) {
                 throw new Exception("Server Unavailable!");
             }
             try {
                 //Log.d("TCP", "C: Sending: '" + message + "'");
                 PrintWriter out = new PrintWriter( new BufferedWriter( new OutputStreamWriter(socket.getOutputStream())),true);
                 String message = Measurements.remove();
                 out.println(message);
                 Thread.sleep(200);
                 Log.d("TCP", "C: Sent.");
                 Log.d("TCP", "C: Done.");
                 connectionAvailable = true;              
             } catch(Exception e) {
                 Log.e("TCP", "S: Error", e);
                 connectionAvailable = false;
             } finally {
                 socket.close();
                 announceNetworkAvailability(connectionAvailable);
             }
         } catch (Exception e) {
             Log.e("TCP", "C: Error", e);
             connectionAvailable = false;
             announceNetworkAvailability(connectionAvailable);
         }
    }
}

...
}

After I close the application the phone works really slow and I guess it is due to thread termination failure.

Does anyone know what is the best way to terminate all threads before terminating the application?

Answer 1

There are several problems in the code sample you posted I will address in order:

1) Thread.stop() has been deprecated for quite some time now, as it can leave dependent variables in inconsistent states in some circumstances. See this Sun answer page for more details. A preferred method of stopping and starting a thread is as follows:

private volatile Thread runner;

public synchronized void startThread(){
  if(runner == null){
    runner = new Thread(this);
    runner.start();
  }
}

public synchronized void stopThread(){
  if(runner != null){
    Thread moribund = runner;
    runner = null;
    moribund.interrupt();
  }
}

public void run(){
  while(Thread.currentThread() == runner){
    //do stuff which can be interrupted if necessary
  }
}

2) Your measurements list is accessed by multiple threads (the event thread and your user thread) at the same time without any synchronization. It looks like you don't have to roll your own synchronization, you can use a BlockingQueue.

3) You are creating a new Socket every iteration of your sending Thread. This is a rather heavyweight operation, and only really make sense if you expect measurements to be extremely infrequent (say one an hour or less). Either you want a persistent socket that is not recreated every loop of the thread, or you want a one shot runnable you can 'fire and forget' which creates a socket, sends all relevant data, and finishes. (A quick note about using a persistent Socket, socket methods which block, such as reading, cannot be interrupted by Thread.interrupt(), and so when you want to stop the thread, you must close the socket as well as calling interrupt)

4) There is little point in throwing your own exceptions from within a Thread unless you expect to catch it somewhere else. A better solution is to log the error and if it is irrecoverable, stop the thread. A thread can stop itself with code like (in the same context as above):

public void run(){
    while(Thread.currentThread() == runner){
      //do stuff which can be interrupted if necessary

      if(/*fatal error*/){
        stopThread();
        return; //optional in this case since the loop will exit anyways
      }
    }
  }

Finally, if you want to be sure a thread exits with the rest of your application, no matter what, a good technique is to call Thread.setDaemon(true) after creation and before you start the thread. This flags the thread as a daemon thread, meaning the VM will ensure that it is automatically destroyed if there are no non-daemon threads running (such as if your app quits).

Obeying best practices with regards to Threads should ensure that your app doesn't hang or slow down the phone, though they can be quite complex :)

Answer 2

Actually, you don't need the "runner" variable as described above, something like:

while (!interrupted()) {
    try {
        Thread.sleep(1000);
    } catch (InterruptedException ex) {
        break;
    }
}

But generally, sitting in a Thread.sleep() loop is a really bad idea.

Look at the AsyncTask API in the new 1.5 API. It will probably solve your problem more elegantly than using a service. Your phone is getting slow because the service never shuts down - there's nothing that will cause the service to kill itself.

Answer 3

I have tried Thread.interrupt() and then i checked whether thread is alive or not using thread.isAlive(),it still shows that thread is alive by returning true value for isAlive,its not correct way of killing the thread.