Android - avoiding memory leak in AsyncTask when passing a button instance

Question

I have a class which extends AsyncTask. When called, this task will download a video to internal storage and will in turn update a progress indicator. When the task is done, it will change the download button to a downloaded button (I'm using abdularis AndroidButtonProgress).

The procedure is working well, however I have a field for the download button and it's being highlighted as a memory leak:

public class DownloadHandler extends AsyncTask<Object, Integer, String> {

    private DownloadButtonProgress downloadButton; // This field leaks a context object

    private WeakReference<Context> context;

    Episode episode;

    int totalSize;


    public DownloadHandler(Context context) {
        this.context = new WeakReference<> (context);
    }

    @Override
    protected String doInBackground(Object... params) {
        episode = (Episode) params[0];
        Context context = (Context) params[1];
        downloadButton = (DownloadButtonProgress) params[2];

        String urlString = "https://path.to.video.mp4";

        try {
            URL url = new URL(urlString);

            URLConnection ucon = url.openConnection();
            ucon.setReadTimeout(5000);
            ucon.setConnectTimeout(10000);
            totalSize = ucon.getContentLength();

            InputStream is = ucon.getInputStream();
            BufferedInputStream inStream = new BufferedInputStream(is, 1024 * 5);

            String fileName = episode.getFilename() + ".mp4";
            File file = new File(String.valueOf(context.getFilesDir()) + fileName);

            if (file.exists()) {
                file.delete();
            }
            file.createNewFile();

            FileOutputStream outStream = new FileOutputStream(file);
            byte[] buff = new byte[5 * 1024];

            int len;
            long total = 0;
            while ((len = inStream.read(buff)) != -1) {
                total += len;
                if (totalSize > 0) {
                    publishProgress((int) (total * 100 / totalSize));
                }
                outStream.write(buff, 0, len);
            }

            outStream.flush();
            outStream.close();
            inStream.close(); 

            return "Downloaded";

        } catch (Exception e) {
            e.printStackTrace();
            return "Not downloaded";
        }
    }

    @Override
    protected void onProgressUpdate(Integer... progress) {
        int downloadedPercentage = progress[0];
        downloadButton.setCurrentProgress(downloadedPercentage);
    }

    @Override
    protected void onPostExecute(String result) {
        if (!result.equals("Downloaded")) {
            Log.d(TAG, "onPostExecute: ERROR");
        } else {
            downloadButton.setFinish();

            // Save to Room (this is why I pass context as a weak reference)
            AppDatabase db = AppDatabase.getDbInstance(context.get().getApplicationContext());
            // ....
        }

    }
}

When I call the DownloadHandler from the fragment I do it like this:

DownloadHandler downloadTask = new DownloadHandler(getActivity());
downloadTask.execute(episode, getActivity(), downloadButton);

I'm passing the download button in the execute method, but I need it to be available to the other methods in the DownloadHandler class (onProgressUpdate(), onPostExecute()) so I made it a field.

I tried passing it in the constructor as a weak reference as I do the context but I got an error saying I can't cast the downloadButton to WeakReference.

How can I have make the downloadButton available to all the methods in the Download Handler but avoiding the memory leak?

score 0 · Accepted Answer · answered May 30 '21 at 15:51

You should pass the download button as a constructor dependency and wrap it in a weak reference as you've done with context.

I think it may have thrown a ClassCastException because you attempted to force cast it from doInBackground() and the download button from the host of your AsyncTask was a weak reference of the view.

Minor modification to your existing code should work fine:

public class DownloadHandler extends AsyncTask<Object, Integer, String> {

    private WeakReference<DownloadButtonProgress> downloadButton;

    private WeakReference<Context> context;

    Episode episode;

    int totalSize;


    public DownloadHandler(Context context, DownloadButtonProgress button) {
        this.context = new WeakReference<> (context);
        this.downloadButton = new WeakReference<>(button)
    }

    @Override
    protected String doInBackground(Object... params) {
        episode = (Episode) params[0];

        String urlString = "https://path.to.video.mp4";

        try {
            URL url = new URL(urlString);

            URLConnection ucon = url.openConnection();
            ucon.setReadTimeout(5000);
            ucon.setConnectTimeout(10000);
            totalSize = ucon.getContentLength();

            InputStream is = ucon.getInputStream();
            BufferedInputStream inStream = new BufferedInputStream(is, 1024 * 5);

            String fileName = episode.getFilename() + ".mp4";
            File file = new File(String.valueOf(context.get().getFilesDir()) + fileName);

            if (file.exists()) {
                file.delete();
            }
            file.createNewFile();

            FileOutputStream outStream = new FileOutputStream(file);
            byte[] buff = new byte[5 * 1024];

            int len;
            long total = 0;
            while ((len = inStream.read(buff)) != -1) {
                total += len;
                if (totalSize > 0) {
                    publishProgress((int) (total * 100 / totalSize));
                }
                outStream.write(buff, 0, len);
            }

            outStream.flush();
            outStream.close();
            inStream.close(); 

            return "Downloaded";

        } catch (Exception e) {
            e.printStackTrace();
            return "Not downloaded";
        }
    }

    @Override
    protected void onProgressUpdate(Integer... progress) {
        int downloadedPercentage = progress[0];
        if (downloadButton.get() != null) {
            downloadButton.get().setCurrentProgress(downloadedPercentage);
        }
    }

    @Override
    protected void onPostExecute(String result) {
        if (!result.equals("Downloaded")) {
            Log.d(TAG, "onPostExecute: ERROR");
        } else {
            if (downloadButton.get() != null) {
                downloadButton.get().setFinish();
            }

            // Save to Room (this is why I pass context as a weak reference)
            AppDatabase db = AppDatabase.getDbInstance(context.get().getApplicationContext());
            // ....
        }
    }
}

now you could invoke it like this (notice the use of context's weak ref in doInBackground):

DownloadHandler downloadTask = new DownloadHandler(getActivity(), downloadButton);
downloadTask.execute(episode);

With that said, it's still not neat because of all the null checks that you'll need which create a lot of poor readability issues imo so to avoid the leaks make sure you use AsyncTask#cancel() API to cancel any ongoing task when the activity is destroyed and then you could drop all the weak references from you implementation (assuming activity re-creation takes care of the state again)

Also in the long run you might wanna check out better asynchronous APIs like co-routines or RxJava as AsyncTask has been deprecated.

Thank you this worked. Will definitely look at other options but for now this fixed it. — cesarcarlos, May 30 '21 at 16:02

Android - avoiding memory leak in AsyncTask when passing a button instance

1 Answers1