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?
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.