File downloader using Java, multithreading and HTTP Range request

I'm making a file downloader using Java. The project is on GitHub; I have included the most relevant excerpts here.

I separate the file into parts and download them using HTTP Range request, with each part being handled by a thread. All of the threads share an object, which is used to keep track of the progress and print the progress bar. The client is something like this:

public class DownloadThread implements Runnable {

    // Constructors and helper methods go here
    // ...

    public void downloadToFile(HttpURLConnection conn) throws IOException {
        //...

        while (mDownloadedSize < contentLength) {
            /*
             * Code to get the data from the input stream goes here.
             */

            // After getting the data, the thread updates the progress and
            // notifies the other threads.
            synchronized (mProgress) {
                // mProgress is the object used to update the progress
                // result is the number of bytes read from the input stream
                mProgress.downloadedCount += result;

                // ...
                mProgress.updateProgressBar();

                mProgress.notifyAll();
            }
        }
    }

    // ...
}

This is the main download class, which is used to manage the smaller download thread objects.

```
public class Download implements Runnable {

// Other functions go here.
// ...

@Override
public void run() {
try {
// Get the file name and create the URL object
String fileName = new File(mUrl).getName();
URL url = new URL(mUrl);

// Check the validity of the URL
HttpResult result = checkURLValidity(url);
long contentSize = result.contentLength;
int responseCode = result.responseCode;

if (contentSize == -1 || responseCode != 200) {
String errMessage = "Error while checking URL validity!";

Naming convention, access level and exception handling

Java's naming convention do not follow the hungarian notation and thus do not include a leading m for member variable as in mProgress which therefore should probably get renamed to progress. You should also make use of encapsulation and avoid accessing fields directly:

while (progress.mURLVerifyResult.responseCode == 0
                && progress.ex == null) {

If you insist in accessing fields rather then getter/setter methods internally, then at least declare these fields as package-private instead of public:

public class Progress {
    public HttpResult mURLVerifyResult;
    public Exception ex;
    public boolean downloadFinished;
    public boolean joinPartsFinished;

    public long downloadedCount;
    public long time;
    public long sizeChange;
    public long percentageCount;

    public Instant startDownloadTimeStamp;
    ...
}

In additon to that, specifying percentageCount as long is probably a waste of memory as under normal circumstances this should never exceed 100, therefore int or short are probably enough.

Instead of ignoring a caught exception:

catch (IOException ex) {
    // If failed to delete then just ignore the exception.
    // What can we do?
}

you should at least log the exception or write it to the error stream. If you log it on warn-, debug- or trace-level is up to you, but at least log it somewhere.

Something like this however, does not make any sense:

try {
    downloadParts = startDownloadThreads(url, contentSize, mPartsCount, mProgress);
} catch (RuntimeException ex) {
    throw ex;
}

Either do something with the exception or don't catch the runtime exception at all at this place.

Don't repeat yourself (DRY) - Refactor synchronized progress logic

As the main method contains the synchronized progress logic three times, this could be refactored to something like:

private static void synchronizeProgress(Progress progress, boolean condition, ProgressHandler handler) {
    synchronized (progress) {
        // Wait until all parts finish joining or an exception is thrown.
        while (condition && progress.ex == null) {
            progress.wait();
        }

        if (progress.ex == null) {
            handler.handle(progress.mURLVerifyResult);
        } else {
            // Else print the error message and exit.
            printErrorMessage(progress.ex);
        }
    }
}

Where the ProgressHandler is a simple interface

public interface ProgressHandler {
    void handle(HttpResult verifyResult);
}

In public static void main(...) the current sychronized(progress) blocks could now be refactored to

synchronizeProgress(progress, 
                    progress.mURLVerifyResult.responseCode == 0, 
                    (verifyResult) -> {
    // If no exception was thrown, URL verification succeeds.
    System.out.println("Response code: "+ verifyResult.responseCode);
    System.out.println("Fize size: "
                       + Utility.readableFileSize(verifyResult.contentLength));
});
...
synchronizeProgress(progress,
                    !progress.downloadFinished,
                    (verifyResult) -> {
    // If no exception was thrown. the file was downloaded successfully.
    downloadFinish = Instant.now();
    double downloadTime = ((double) (Duration.between(start,
                    downloadFinish).toMillis())) / 1000;

    System.out.println("\n\nTotal download time: " + downloadTime);
});
...
synchronizeProgress(progress,
                    !progress.joinPartsFinished,
                    (verifyResult) -> {
    // If no exception is thrown, parts joining succeeds.
    Instant joinFinishedTime = Instant.now();
    double joinTime = ((double) (Duration.between(downloadFinish,
                    joinFinishedTime).toMillis())) / 1000;

    System.out.println("Total join time: " + joinTime);
});

Logical Issues

You set an synchronization point while waiting for the validation of the URI to finish. This does not make sense to me as all the download threads will use the same URI and thus need to be verified only once before passing the URI to the Download object.

You defined Download as a runnable but yet do not really make use of the thread as you have only one URL to download defined. Also, Progress is only able to handle one download URL at a time. Maybe the Progress class can therefore be managed by the Download class directly.

Consider replacing wait and notifyAll with Java concurrent utils

As you have some mProgress.wait() and mProgress.notifyAll() invocations throughout your code, which are inherited from Object, this should probably get replaced with one of the Java concurrent utility classes like Lock, Condition, CountDownLatch or CyclicBarrier.

The JavaDoc for CountDownLatch f.e. states:


A synchronization aid that allows one or more threads to wait until a set of operations being perform

StackExchange Code Review Q#135816, answer score: 5