Is this Runnable safe from memory leak?
Asked Answered
D

4

7

I am a total beginner in Java and have created a simple Java Android snippet where in a Runnable after 1,5 seconds I change the TextView from Hello World to Hola Mundo. It works flawlessly, basically a WeakReference should prevent this memory leak from happening right? I have a doubt if there's absolutely no memory leak whenever device orientation occurs. I would love to check this but can't manage to change orientation in my emulated Android.

This is the code:

package com.example.helloworld;

import android.app.Activity;
import android.os.Bundle;
import android.os.Handler;
import android.widget.TextView;
import android.util.Log;
import java.lang.ref.WeakReference;

public class HelloWorldActivity extends Activity
{
    private Handler h = new Handler();
    private static TextView txtview;
    /** Called when the activity is first created. */
    @Override
    public void onCreate(Bundle savedInstanceState)
    {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.main);
        txtview = (TextView) findViewById(R.id.mainview);

        h.postDelayed(new WeakRunnable(txtview),1500);
    }

    private static final class WeakRunnable implements Runnable {
        private final WeakReference<TextView> mtextview;

        protected WeakRunnable(TextView textview){
            mtextview = new WeakReference<TextView>(textview);
        }

            @Override
            public void run() {
                TextView textview = mtextview.get();
                if (textview != null) {
                    txtview.setText("Hola Mundo");
                    textview = null; // No idea if setting to null afterwards is a good idea
                }
                Log.d("com.example.helloworld", "" + textview);
            }
    }           

}

EDIT

It's safe from memory leaks but a few answers were also concerned with UI thread blocking. In fact this code runs the Handler in the main (UI) thread. To spawn a new thread I'm spawning a thread manually as follows:

package com.example.helloworld;

import android.app.Activity;
import android.os.Bundle;
import android.os.Handler;
import android.widget.TextView;
import android.util.Log;
import java.lang.ref.WeakReference;

public class HelloWorldActivity extends Activity
{

    private static TextView txtview;
    /** Called when the activity is first created. */
    @Override
    public void onCreate(Bundle savedInstanceState)
    {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.main);
        txtview = (TextView) findViewById(R.id.mainview);

        Thread t = new Thread(new WeakRunnable(txtview));
        t.start();
    }

    private static final class WeakRunnable implements Runnable {
        private final WeakReference<TextView> mtextview;

        protected WeakRunnable(TextView textview){
            mtextview = new WeakReference<TextView>(textview);
        }

            @Override
            public void run() {
                TextView textview = mtextview.get();
                if (textview != null) {
                    /*
                    try {
                        Thread.sleep(1500);
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                    */
                    txtview.setText("Hola Mundo");
                    textview = null;
                }
                Log.d("com.example.helloworld", "" + Thread.currentThread().getName()); // Outputs "Thread-<num>" if not running on UI thread
            }
    }           

}

The issue now is that I can't seem to delay the spawned thread in any way, otherwise it works.

This:

try {
    Thread.sleep(1500);
} catch (InterruptedException e) {
    e.printStackTrace();
}

makes the app quit itself and I don't get why. Something tells me I'm delaying it the wrong way.

EDIT2

Thanks to the link @EugenMatynov give me: update ui from another thread in android I understood why the app quitted. It all comes down to the reason You can't call UI methods from threads other than the main thread. and it's bad practice to update the UI from another thread.

Deltoid answered 17/4, 2015 at 8:36 Comment(2)
clean the handler's queue on onPause, and you are 100% leak free. In this case.Iridectomy
Isn't there a memory leak in virtue of the static TextView field?Calomel
C
4

I think your code is leak free if you use :

private static Handler h = new Handler(); 

or

txtview.postDelayed(new WeakRunnable(txtview),1500);

because you have stored the view as a WeakReference. the method:

txtview.postDelayed(new WeakRunnable(txtview),1500);

simply call main handler of the UI thread so if the activity is destroyed the view is null and the runnable dose nothing.

also because of the weakreference the activity can be garbage collected because there is no strong reference to it.

Celtuce answered 17/4, 2015 at 8:52 Comment(4)
After you accepted my answer again I reviewed my answer and your question and found a bug in my answer, my answer is only valid if you use private static Handler h = new Handler(); or txtview.postDelayed(new WeakRunnable(txtview),1500); you can accept blackbeltt answe or let me edit it. also note that running a runnable by delay is different from sleeping on main thread. because I think people make you confusing :-)Celtuce
What's the difference between static and non-static version?Deltoid
Got it, nice explanation. I'm rather new to Java so I have to click some concepts. I opted for txtview.postDelayed(new WeakRunnable(txtview),1500); since it looks easier and takes relatively less visual space. Feel free to edit your answer :)Deltoid
static => member of class - non-static => member of objectsCeltuce
I
7

I have a doubt if there's absolutely no memory leak whenever device orientation occurs.

It could be. For 1.5seconds. After the queue is emptied the handler can be garbage collected, and also the old Activity. To be safe override onPause, and call handler.removeCallbacks(null); to clear the Handler's queue

Iridectomy answered 17/4, 2015 at 8:50 Comment(1)
In my case, I had to call handler.removeCallbacksAndMessages(null);, because as of the android version 23 implementation, removeCallbacks(); does nothing when you pass null as a parameter(see MessageQueue's removeMessages();).Allodial
C
4

I think your code is leak free if you use :

private static Handler h = new Handler(); 

or

txtview.postDelayed(new WeakRunnable(txtview),1500);

because you have stored the view as a WeakReference. the method:

txtview.postDelayed(new WeakRunnable(txtview),1500);

simply call main handler of the UI thread so if the activity is destroyed the view is null and the runnable dose nothing.

also because of the weakreference the activity can be garbage collected because there is no strong reference to it.

Celtuce answered 17/4, 2015 at 8:52 Comment(4)
After you accepted my answer again I reviewed my answer and your question and found a bug in my answer, my answer is only valid if you use private static Handler h = new Handler(); or txtview.postDelayed(new WeakRunnable(txtview),1500); you can accept blackbeltt answe or let me edit it. also note that running a runnable by delay is different from sleeping on main thread. because I think people make you confusing :-)Celtuce
What's the difference between static and non-static version?Deltoid
Got it, nice explanation. I'm rather new to Java so I have to click some concepts. I opted for txtview.postDelayed(new WeakRunnable(txtview),1500); since it looks easier and takes relatively less visual space. Feel free to edit your answer :)Deltoid
static => member of class - non-static => member of objectsCeltuce
W
2

h.postDelayed(new WeakRunnable(txtview),1500); I think it will be blocking UI Thread. here is a good sample for memory leak. https://github.com/badoo/android-weak-handler

Wharfinger answered 17/4, 2015 at 8:42 Comment(8)
can you add the relevant code in answer and where the code in question canbe improved?Bridgettebridgewater
It will not block UI, as well code is exactly same code that written in blog related to library techblog.badoo.com/blog/2014/08/28/android-handler-memory-leaksBotheration
That's weird. I thought h.postDelayed(new WeakRunnable(txtview),1500); basically started the newly created thread private Handler h = new Handler(); outside of UI threadDeltoid
@EugenMartynov When executing Thread.currentThread().getName() I get it's on the main (UI) thread. So yeah, if it takes much time it will eventually block UI.Deltoid
It is bad practice to change UI elements out of Main thread #7006256. Sure default Handler runs on Main thread but posting delayed runnable doesn't block ui. Also setting text should be quite fast so user will not notice anythingBotheration
@EugenMartynov Thanks I got it. Does it mean that if I delay the runnable for a few minutes and in the meantime I change another part of the UI it won't block right?Deltoid
@gizko if I understood you correctly - right. Delayed code will be not executed on scheduled thread. So this thread resource can be used by another activityBotheration
@EugenMartynov Perfect, that's what I needed to know. Your link helped me click the concept.Deltoid
O
1

Please do this, otherwise you will be blocking UIThread and it is not recommended. To do this, you can also use a TimerTask, check it here: http://developer.android.com/reference/java/util/TimerTask.html

import android.widget.TextView;
import android.util.Log;
import java.lang.ref.WeakReference;

public class HelloWorldActivity extends Activity
{
    private Handler h = new Handler();
    private static TextView txtview;

    /** Called when the activity is first created. */
    @Override
    public void onCreate(Bundle savedInstanceState)
    {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.main);
        txtview = (TextView) findViewById(R.id.mainview);        

        h.postDelayed(new Runnable() {
           @Override
           public void run() {
              changeText();
           }
        }, 1500);
    }

    public void changeText(){
       txtview.setText("Hola mundo.");
       h.removeCallbacksAndMessages(null);
    }          

}

By the way, you can change orientation in your emulator this way: Ctrl+F12

Orontes answered 17/4, 2015 at 8:57 Comment(4)
Non static inner class can create memory leaks as your code exactly does it:-)Celtuce
I have edited the code, maybe removing callbacks and messages will make the code less susceptible to cause memory leak. What do you think?Orontes
your leak is caused by using anonymous new Runnable(), actually the OP code is correct, you can remove callbacks at onPause but may be the OP dose not like to do that because for example the user press the home button and after 2s he back to app so he wants to see the change.Celtuce
Unfortunately I have a Chromebook converted to Linux and have no F12 key :/ I have updated my question and spawning a new thread helps avoiding blocking the UIThread, but I can't seem to be able to delay its execution without the app quitting itself.Deltoid

© 2022 - 2024 — McMap. All rights reserved.