Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented Parallel Download of Forms using Multi-Threading and Locks #60

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.function.Supplier;

import javax.annotation.Nullable;
Expand All @@ -46,6 +48,8 @@ public ServerFormDownloader(FormSource formSource, FormsRepository formsReposito
this.clock = clock;
}

private static Lock lock = new ReentrantLock();

@Override
public void downloadForm(ServerFormDetails form, @Nullable ProgressReporter progressReporter, @Nullable Supplier<Boolean> isCancelled) throws FormDownloadException {
Form formOnDevice;
Expand Down Expand Up @@ -94,8 +98,8 @@ private void processOneForm(ServerFormDetails fd, OngoingWorkListener stateListe
// get the xml file
// if we've downloaded a duplicate, this gives us the file
fileResult = downloadXform(fd.getFormName(), fd.getDownloadUrl(), stateListener, tempDir, formsDirPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this method is thread safe? If so, how?


// download media files if there are any
lock.lock();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've acquired the lock at the beginning of the process and released the lock at the end of the download process. What this essentially does is:

  • A new thread acquires the lock to download form media.
  • The thread downloads the form media by making a web request.
  • The thread releases the lock.

How is different than synchronous processing if the other threads are anyways waiting for the lock to be released while the download process is on?

if (fd.getManifest() != null && !fd.getManifest().getMediaFiles().isEmpty()) {
FormMediaDownloader mediaDownloader = new FormMediaDownloader(formsRepository, formSource);
newAttachmentsDetected = mediaDownloader.download(fd, fd.getManifest().getMediaFiles(), tempMediaPath, tempDir, stateListener);
Expand Down Expand Up @@ -138,6 +142,7 @@ private void processOneForm(ServerFormDetails fd, OngoingWorkListener stateListe

try {
installEverything(tempMediaPath, fileResult, parsedFields, formsDirPath, newAttachmentsDetected);
lock.unlock();
} catch (FormDownloadException.DiskError e) {
cleanUp(fileResult, tempMediaPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You haven't released the lock in catch. This could lead to a deadlock if an exception occurs. This also needs to be handled in other catch blocks as well.

throw e;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import static java.util.Collections.emptyMap;

import android.os.AsyncTask;
import android.os.Handler;
import android.os.Looper;

import org.odk.collect.android.R;
import org.odk.collect.android.application.Collect;
Expand All @@ -30,6 +32,8 @@
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

/**
* Background task for downloading a given list of forms. We assume right now that the forms are
Expand All @@ -53,34 +57,48 @@ public DownloadFormsTask(FormDownloader formDownloader) {
protected Map<ServerFormDetails, FormDownloadException> doInBackground(ArrayList<ServerFormDetails>... values) {
HashMap<ServerFormDetails, FormDownloadException> results = new HashMap<>();

ExecutorService executorService = Executors.newCachedThreadPool();
int index = 1;
for (ServerFormDetails serverFormDetails : values[0]) {
try {
String currentFormNumber = String.valueOf(index);
String totalForms = String.valueOf(values[0].size());
publishProgress(serverFormDetails.getFormName(), currentFormNumber, totalForms);

formDownloader.downloadForm(serverFormDetails, count -> {
String message = getLocalizedString(Collect.getInstance(), R.string.form_download_progress,
serverFormDetails.getFormName(),
String.valueOf(count),
String.valueOf(serverFormDetails.getManifest().getMediaFiles().size())
);

publishProgress(message, currentFormNumber, totalForms);
}, this::isCancelled);

results.put(serverFormDetails, null);
FormEventBus.INSTANCE.formDownloaded(serverFormDetails.getFormId());
} catch (FormDownloadException.DownloadingInterrupted e) {
return emptyMap();
} catch (FormDownloadException e) {
results.put(serverFormDetails, e);
FormEventBus.INSTANCE.formDownloadFailed(serverFormDetails.getFormId(), e.getMessage());
}
String currentFormNumber = String.valueOf(index);
String totalForms = String.valueOf(values[0].size());
publishProgress(serverFormDetails.getFormName(), currentFormNumber, totalForms);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this process is not synchronous now, currentFormNumber will return wrong data when used in a concurrent threaded system.


Thread thread = new Thread() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we limiting resource consumption on this? If there are a large number of forms let's say 1000 forms, this would open 1000 threads which is not exactly resource friendly. We should provide a configurable way to limit the number of threads.

public void run() {

try {
formDownloader.downloadForm(serverFormDetails, count -> {
String message = getLocalizedString(Collect.getInstance(), R.string.form_download_progress,
serverFormDetails.getFormName(),
String.valueOf(count),
String.valueOf(serverFormDetails.getManifest().getMediaFiles().size())
);

publishProgress(message, currentFormNumber, totalForms);

}, DownloadFormsTask.this::isCancelled);
} catch (FormDownloadException e) {
results.put(serverFormDetails, e);
FormEventBus.INSTANCE.formDownloadFailed(serverFormDetails.getFormId(), e.getMessage());
throw new RuntimeException(e);
}

new Handler(Looper.getMainLooper()).post(new Runnable() {
@Override
public void run() {
results.put(serverFormDetails, null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HashMaps are not thread safe.

FormEventBus.INSTANCE.formDownloaded(serverFormDetails.getFormId());
}
});
}
};

executorService.submit(thread);

index++;
}
executorService.shutdown();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executorService may shut down before all the threads have completed work, it is not a blocking method. In such a scenario results might not contain all the forms data.


return results;
}
Expand Down