Ending threads in Android - Drastic problems

Question

I'm still pretty confused as to when / how to end threads in my Surfaceview application and was hoping someone could explain.

Currently, I am using this code:

Log.v("Destroyed","Surface Destroyed");

preThread.setRunning(false);  
boolean retry = true;        
while (retry) {
    try {
        preThread.join();
        retry = false;
    } catch (InterruptedException e) {
    }
}

The above code sits in my surfaceDestroyed method - firstly, is this correct?

In my surfaceCreated method I have the following code which should check to see if the thread still exists or has been stopped and if it's been stopped, then re-start it:

if (runthread==false){

if (preThread.getState()==Thread.State.TERMINATED){
    preThread = new MainThread(thisholder, thiscontext, thishandler);}
else {}
    preThread.setRunning(true);
    preThread.start();
    }

It seems to act really strange. Here is what I get:

*) When I first install the game and run it, through my logging, it says that the thread already exists, If I then press the back key, surfaceDestroyed is run but when I go back into the activity, again it says that the thread already exists.

*) If I press the home key, then surfaceDestroyed is run and when I go back into the activity it says that the thread was previously destroyed and starts a new one.

*) If I kill the activity using DDMS, surfaceDestroyed isn't run and when I go back into the activity it says that the thread already exists.

If I'm thinking straight, then the third scenario is the only one that seems to make sense.

I'm clearly doing something drastically wrong. The main problem is this:

If I hit the home key during the game and then end the app via DDMS in Eclipse, restart the app and hit the back key twice in quick succession (once, to go back to the previous activity, then again to get back to the splash screen) - the app force-closes and I get a "Fatal exception: Thread 12" in logcat. I have to assume this is because my thread is never ending and is trying to be-restarted? I'm not sure.

I've been trying to figure this out for what seems like an age so I really hope someone can explain what I'm doing wrong!

Many thanks!

Edit. Logcat output.

my Run() method:

    public void run(){

    //Main Loop

    while (runthread){

    Log.v("tracking","runthread is: "+runthread);                             //This should only be logged while this loop is running

        timestart = System.currentTimeMillis();                             //Get time at start of loop for FPS calc

        try{c=mySurfaceHolder.lockCanvas();                                 //Set Canvas to locked

            synchronized(mySurfaceHolder){


                if (c==null){Log.v("Stop","Canvas is null for some reason - exiting, "+c+" - see?!!!");}


                framesskipped = 0;                                              // resetting frames skipped
        doDraw(c);                                                      //Draw to the screen
        updateMenu();
        }
        }
        finally{
            if (c != null){

            mySurfaceHolder.unlockCanvasAndPost(c);                     //Post canvas

            }
        }

        //work out timings

            timeend = System.currentTimeMillis();                       //get end time for current frame (for FPS) 
            frametime = timeend-timestart;                              //Set the frametime variable to the time the frame took to render & update (end time - start time)
            sleepfor = (int) (33-frametime);                            // this is the time that the thread will sleep for if <target time


            if (sleepfor>0){                                            // If the 'sleepfor' variable is >0 then set the thread to sleep for it's value (expressed in ms)

                try {
                    OptionsThread.sleep(sleepfor);                      //send thread to sleep for value of sleepfor (determined above).
                } catch (InterruptedException e) {}                     //in case of exception
            }                                                           //close if statement


                while (sleepfor<0 && framesskipped<maxframesskipped){   //if sleepfor is < 0 (ie, frame took longer to render than target time and the maxframesskipped has not reached it's limit)
                    updateMenu();                                       //Update animation variables without rendering to the screen while these conditions are met 
                    sleepfor+=33;                                       //time to sleep plus the time frame took to render
                    framesskipped++;                                    //add one to framesskipped variable so this only skips a certain number of frames
                    }
                }
            }

New Logcat output showing nullPointerException and output of logging. runThread is never logged as false so I'm not sure how the line that logs canvas as null is reached!

Thanks

Edit:

OK, I've completely started from scratch and re-written the whole class- it's very much a stripped-down version of what I had before and here is the whole class:

    import android.content.res.Resources;
    import android.content.Context;
    import android.graphics.Bitmap;
    import android.graphics.BitmapFactory;
    import android.graphics.Canvas;
    import android.os.Handler;
    import android.util.Log;
    import android.view.MotionEvent;
    import android.view.Surface;
    import android.view.SurfaceHolder;
    import android.view.SurfaceView;

public class OptionsScreen extends SurfaceView implements
  SurfaceHolder.Callback {   

    //Create Variables

    private SurfaceHolder thisHolder;
    private Context thisContext;
    private Handler thisHandler;
    private preThread thread;
    private Bitmap background;
    private Resources res;
    private Context myContext;
    private Handler myHandler;


    private Canvas c;
    //      thisholder = getHolder();


    public OptionsScreen(Context context) {
    super(context);

    myContext=context;                                  //This is the context passed into this constructor (this)
    thisHolder = getHolder();                           //Get surface holder
    thisHandler=getHandler();                           //Get Handler
    thisContext = getContext();                         //Get context
    res=getResources();                                 //Get resource

    //add the callback surface holder
    getHolder().addCallback(this);
    //make focusable
    setFocusable(true);
    //create new thread
    thread = new preThread(thisHolder, thisContext, thisHandler);
    //create bitmaps from resources
    background = BitmapFactory.decodeResource(res, R.drawable.sky);
}

 @Override
 public void surfaceChanged(SurfaceHolder holder, int format, int width, int height) {

    Log.v("check","surfaceChanged run");

 }

 @Override
 public void surfaceCreated(SurfaceHolder holder) {

    Log.v("check","surfaceCreated run"+thread.getState());

    int height = this.getHeight();
    int width = this.getWidth();

    if(thread.getState()==Thread.State.TERMINATED){             //Has thread been stopped previously? could happen if the home key is pressed
        Log.v("check","Thread still exists!!!! - Starting a new one. "+thread.getState()); 
        thread = new preThread(thisHolder, thisContext, thisHandler);

    }

    thread.setRunning(true);
            thread.start();

    Log.v("check","Thread - "+thread.getState());

 }

 @Override
 public void surfaceDestroyed(SurfaceHolder holder) {

      Log.v("check","surfaceDestroyed run"+thread.getState());

      thread.setRunning(false);                     //Set to false to exit run() method

     boolean retry = true;                          //Shut off rendering thread
          while (retry) {
            try {
                thread.join();
                retry = false;
              } catch (InterruptedException e) {
            // try again shutting down the thread

           }

          }

 }

 @Override
 public boolean onTouchEvent(MotionEvent event) {
     Log.v("check","Surface Touched");
    Log.v("check","Thread - "+thread.getState());
     // System.exit(0);

    return super.onTouchEvent(event);
 }

 @Override
 protected void onDraw(Canvas canvas) {

    // if (canvas!=null){

        canvas.drawBitmap(background, 0, 0, null);
        Log.v("Stop","Canvas is "+canvas);
     }
 }








 //*******************************************************************
 //**                           run loop                            **
 //*******************************************************************

protected class preThread extends Thread {

    private SurfaceHolder mySurfaceHolder;
    private Context myContext;

    public preThread(SurfaceHolder surfaceholder, Context context, Handler handler) {               //Constructor

        mySurfaceHolder=surfaceholder;
        myContext=context;
        res = myContext.getResources();


    }



         // flag
         private boolean running;
         public void setRunning(boolean running) {
          this.running = running;
         }

         @Override
         public void run() {

                      while (running) {

            try{c=mySurfaceHolder.lockCanvas();                                 

            synchronized(mySurfaceHolder){


                Log.v("check","Drawing!!");

        onDraw(c);                                                      

        }
        }
        finally{
            if (c != null){

            mySurfaceHolder.unlockCanvasAndPost(c);                     

            }
        }





          }
         }
        }
}

Answer 1

Threads are pretty difficult to manage, but after some hit and trial i think i have come up with a scheme which works correctly most of the time. To End a Thread

 if(m_hThread != null)
        {
            try 
            {
                m_bThread = false; // m_bThread is the while condition of the thread
                m_hThread.interrupt(); // incase the thread is in sleep
                m_Thread.join(); // This call blocks and waits for thread to end
                m_hThread = null;  

            } catch (InterruptedException e) {

                e.printStackTrace();
            }
        }

For re-creating thread

 if(m_hThread == null) 
     {   
         m_bThread = true; //while condition of thread
         StartNewThread(); 
     }

In your implementation, No need to retry Thread.join, either it will join in the first try or it will block until the thread joins. As for your cases only the first case seems to be weird, where you find the thread is already running, this can't possibly be true. Second and third make complete sense to me, and work like they should. When the user clicks Home button surfaceDestroyed is invoked and thread is Terminated.

thread continues as long as m_bThread is true,

 while(m_bThread) //b
    {
      // Continuous Thread operations...
    }

m_hThread is simply preThread in your code, and m_Thread is a also m_hThread just a typing mistake here.