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

Question

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?

Daffodil

Check out this blog post for a lot more information on this subject! – Ial

Memory leaks caused by garbage collection... This is enough to demonstrate how Java is inconsistent and badly designed – Stepdame

Answer 1

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

Inshore

Thanks for your answer. How can I fix this issue? – Daffodil

Thank you very much for this answer. The lint warning about static handlers always mystified me and after searching around for an hour or so on this topic your answer explained it well and even gave a source of Romain Guy, if I could give it a second upvote I would – Burushaski

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. – Toyatoyama

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. – Inshore

OK. Thanks for re-iterating that the Handler class must be static. My Handler calls functions within the outer class depending on the message received, so really my entire service (outer class) will be massively affected if the inner Handler class needs to be static. – Toyatoyama

Tomasz, what happens when mService.get() == null ? What happens to the message ? Is it actually possible to receive a null from mService.get() ? – Toyatoyama

@SomeoneSomewhere mSerivce is a WeakReference. get() will return null when referenced object was gc-ed. In this case, when service is dead. – Inshore

@Tomasz Thanks for writing this answer. It was really useful. – Grandnephew

@Macarse Dear Macarse. Can u edit u answer and show how u declared UDPListenerService? – Topgallant

Service is same as in the question. Only have to add handleMesage(msg) method in it. – Inshore

@Macarse can i avoid to extent Handler class? i was try to subclass and using it by public WeakHandler uaReceiverHandler = new WeakHandler(phonePadForUsing); but hangleMessage was not fired ( – Topgallant

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));". – Efficacy

@Tomasz Niedabylski I get an error that the final field cannot be assigned (mService is declared final, then we try to assign it in the constructor). Also, if the handler is static, then getApplicationContext() does not work so I can't produce a toast. Any thoughts on either of these issues? – Anderaanderea

Make sure mService is only declared, not assigned at declaration (final mService = null; is a mistake). As for the toast, put the code in service.handleMessage() – Inshore

@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. – Lazes

Android developers use this approach so it can't be that bad, right? developer.android.com/reference/android/app/… – Rodenhouse

Answer 2

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.

Ethben

@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. – Lazes

Answer 3

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);
  }
}

Lazes

Sogger example is great. However, the last method in Myclass should be declared as public Handler getHandler() instead of public void – Livery

It is similar to an answer of Tomasz Niedabylski – Verne

Answer 4

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;
        }
}

Diglot

In here handleMessage method returns true in the end. Could you please explain what exactly this means(the return value true/false)? Thanks. – Heed

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. – Diglot

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. – Pontus

@StuartCampbell: You are correct. See: groups.google.com/forum/#!topic/android-developers/L_xYM0yS6z8 . – Peckham

Answer 5

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;
        }
    }

    ...

}

Lanitalank

Answer 6

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

Reiche

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. – Lightly

Answer 7

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.

Scutellation

Answer 8

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()) {
/////
}

}

Got