314

I'm developing an Android 2.3.3 application with a service. I have this inside that service to communicate with Main activity:

public class UDPListenerService extends Service
{
    private static final String TAG = "UDPListenerService";
    //private ThreadGroup myThreads = new ThreadGroup("UDPListenerServiceWorker");
    private UDPListenerThread myThread;
    /**
     * Handler to communicate from WorkerThread to service.
     */
    private Handler mServiceHandler;

    // Used to receive messages from the Activity
    final Messenger inMessenger = new Messenger(new IncomingHandler());
    // Use to send message to the Activity
    private Messenger outMessenger;

    class IncomingHandler extends Handler
    {
        @Override
        public void handleMessage(Message msg)
        {
        }
    }

    /**
     * Target we publish for clients to send messages to Incoming Handler.
     */
    final Messenger mMessenger = new Messenger(new IncomingHandler());
    [ ... ]
}

And here, final Messenger mMessenger = new Messenger(new IncomingHandler());, I get the following Lint warning:

This Handler class should be static or leaks might occur: IncomingHandler

What does it mean?

2
  • 27
    Check out this blog post for a lot more information on this subject! Jan 18, 2013 at 18:15
  • 2
    Memory leaks caused by garbage collection... This is enough to demonstrate how Java is inconsistent and badly designed
    – Gojir4
    Jun 6, 2018 at 11:30

8 Answers 8

400

If IncomingHandler class is not static, it will have a reference to your Service object.

Handler objects for the same thread all share a common Looper object, which they post messages to and read from.

As messages contain target Handler, as long as there are messages with target handler in the message queue, the handler cannot be garbage collected. If handler is not static, your Service or Activity cannot be garbage collected, even after being destroyed.

This may lead to memory leaks, for some time at least - as long as the messages stay int the queue. This is not much of an issue unless you post long delayed messages.

You can make IncomingHandler static and have a WeakReference to your service:

static class IncomingHandler extends Handler {
    private final WeakReference<UDPListenerService> mService; 

    IncomingHandler(UDPListenerService service) {
        mService = new WeakReference<UDPListenerService>(service);
    }
    @Override
    public void handleMessage(Message msg)
    {
         UDPListenerService service = mService.get();
         if (service != null) {
              service.handleMessage(msg);
         }
    }
}

See this post by Romain Guy for further reference

16
  • 3
    Romain shows that the WeakReference to the outer class is all that's needed - a static nested class is not necessary. I think I'll prefer the WeakReference approach because otherwise the whole outer class drastically changes due to all the 'static' variables I'd need. Jul 18, 2012 at 19:07
  • 37
    If you want to use a nested class, it has to be static. Otherwise, WeakReference doesn't change anything. Inner (nested but not static) class always holds strong reference to outer class. There is no need for any static variables though. Jul 18, 2012 at 21:24
  • 2
    @SomeoneSomewhere mSerivce is a WeakReference. get() will return null when referenced object was gc-ed. In this case, when service is dead. Jul 23, 2012 at 23:02
  • 2
    Note: after making the IncomingHandler static, I was getting the error "The constructor MyActivity.IncomingHandler() is undefined." on the line "final Messenger inMessenger = new Messenger(new IncomingHandler());". The solution is to change that line to "final Messenger inMessenger = new Messenger(new IncomingHandler(this));". Feb 5, 2013 at 3:18
  • 4
    @Someone Somewhere Yeah, Romain's post is wrong in that he missed declaring the inner class static which misses the whole point. Unless he has some ultra cool compiler that automatically converts inner classes to static classes when they don't use class variables.
    – Sogger
    Jan 7, 2015 at 18:22
73

As others have mentioned the Lint warning is because of the potential memory leak. You can avoid the Lint warning by passing a Handler.Callback when constructing Handler (i.e. you don't subclass Handler and there is no Handler non-static inner class):

Handler mIncomingHandler = new Handler(new Handler.Callback() {
    @Override
    public boolean handleMessage(Message msg) {
        // todo
        return true;
    }
});

As I understand it, this will not avoid the potential memory leak. Message objects hold a reference to the mIncomingHandler object which holds a reference the Handler.Callback object which holds a reference to the Service object. As long as there are messages in the Looper message queue, the Service will not be GC. However, it won't be a serious issue unless you have long delay messages in the message queue.

1
  • 14
    @Braj I don't think avoiding the lint warning but still keeping the error is a good solution at all. Unless, as the lint warning states if the handler is not put on your main looper (and you can ensure all the messages pending on it are destroyed when the class is destroyed) then leaking the reference is mitigated.
    – Sogger
    Jan 7, 2015 at 18:29
33

Here is a generic example of using a weak reference and static handler class to resolve the problem (as recommended in the Lint documentation):

public class MyClass{

  //static inner class doesn't hold an implicit reference to the outer class
  private static class MyHandler extends Handler {
    //Using a weak reference means you won't prevent garbage collection
    private final WeakReference<MyClass> myClassWeakReference; 

    public MyHandler(MyClass myClassInstance) {
      myClassWeakReference = new WeakReference<MyClass>(myClassInstance);
    }

    @Override
    public void handleMessage(Message msg) {
      MyClass myClass = myClassWeakReference.get();
      if (myClass != null) {
        ...do work here...
      }
    }
  }

  /**
   * An example getter to provide it to some external class
   * or just use 'new MyHandler(this)' if you are using it internally.
   * If you only use it internally you might even want it as final member:
   * private final MyHandler mHandler = new MyHandler(this);
   */
  public Handler getHandler() {
    return new MyHandler(this);
  }
}
2
  • 2
    Sogger example is great. However, the last method in Myclass should be declared as public Handler getHandler() instead of public void Mar 11, 2015 at 22:36
  • It is similar to an answer of Tomasz Niedabylski
    – CoolMind
    Aug 12, 2015 at 17:51
27

This way worked well for me, keeps code clean by keeping where you handle the message in its own inner class.

The handler you wish to use

Handler mIncomingHandler = new Handler(new IncomingHandlerCallback());

The inner class

class IncomingHandlerCallback implements Handler.Callback{

        @Override
        public boolean handleMessage(Message message) {

            // Handle message code

            return true;
        }
}
4
  • 2
    In here handleMessage method returns true in the end. Could you please explain what exactly this means(the return value true/false)? Thanks.
    – JibW
    Aug 15, 2013 at 14:32
  • 2
    My understanding of returning true is to indicate that you have handled the message and therefore the message should not be passed anywhere else e.g. the underlying handler. That said I couldn't find any documentation and would be happily corrected. Aug 22, 2013 at 9:18
  • 1
    Javadoc says: Constructor associates this handler with the Looper for the current thread and takes a callback interface in which you can handle messages. If this thread does not have a looper, this handler won't be able to receive messages so an exception is thrown. <-- I think the new Handler(new IncomingHandlerCallback()) won't work when there is no Looper attached to the thread, and that CAN be the case. I am not saying its wrong to do so in some cases, I am just saying its not always working as you might expect.
    – user504342
    Oct 20, 2013 at 13:07
  • 1
    @StuartCampbell: You are correct. See: groups.google.com/forum/#!topic/android-developers/L_xYM0yS6z8 .
    – Maxwell175
    Apr 19, 2014 at 20:11
2

With the help of @Sogger's answer, I created a generic Handler:

public class MainThreadHandler<T extends MessageHandler> extends Handler {

    private final WeakReference<T> mInstance;

    public MainThreadHandler(T clazz) {
        // Remove the following line to use the current thread.
        super(Looper.getMainLooper());
        mInstance = new WeakReference<>(clazz);
    }

    @Override
    public void handleMessage(Message msg) {
        T clazz = mInstance.get();
        if (clazz != null) {
            clazz.handleMessage(msg);
        }
    }
}

The interface:

public interface MessageHandler {

    void handleMessage(Message msg);

}

I'm using it as follows. But I'm not 100% sure if this is leak-safe. Maybe someone could comment on this:

public class MyClass implements MessageHandler {

    private static final int DO_IT_MSG = 123;

    private MainThreadHandler<MyClass> mHandler = new MainThreadHandler<>(this);

    private void start() {
        // Do it in 5 seconds.
        mHandler.sendEmptyMessageDelayed(DO_IT_MSG, 5 * 1000);
    }

    @Override
    public void handleMessage(Message msg) {
        switch (msg.what) {
            case DO_IT_MSG:
                doIt();
                break;
        }
    }

    ...

}
0

I am not sure but you can try intialising handler to null in onDestroy()

1
  • 1
    Handler objects for the same thread all share a common Looper object, which they post messages to and read from. As messages contain target Handler, as long as there are messages with target handler in the message queue, the handler cannot be garbage collected.
    – msysmilu
    Nov 25, 2015 at 19:02
0

I'm confused. The example I found avoids the static property entirely and uses the UI thread:

    public class example extends Activity {
        final int HANDLE_FIX_SCREEN = 1000;
        public Handler DBthreadHandler = new Handler(Looper.getMainLooper()){
            @Override
            public void handleMessage(Message msg) {
                int imsg;
                imsg = msg.what;
                if (imsg == HANDLE_FIX_SCREEN) {
                    doSomething();
                }
            }
        };
    }

The thing I like about this solution is there is no problem trying to mix class and method variables.

0

If you're using Kotlin, simply remove the inner keyword when declaring the nested class.

Nested classes in Kotlin are static by default, declaring them with the inner makes them not-static.

Change your nested Handler subclass declaration from

class myService : Service() {

inner class IncomingHandler : Handler(Looper.getMainLooper()) {
/////
}

}

to

class myService : Service() {

class IncomingHandler : Handler(Looper.getMainLooper()) {
/////
}

}

Not the answer you're looking for? Browse other questions tagged or ask your own question.