Variable synchronization in multiple Java Threads

Question

My question is about how to use the block synchronized, my class FileAdapter has a method write that receives the InputStream of the result of an HTTP connection I’m using to download a file, for every kilobyte downloaded and written to disk, it calls the downloaded method of the DownloadReport class instance that it received, for pass on what has already been downloaded.

In another Thread, which is printing the output to the user, it calls the updateProgress method, also of the DownloadReport class, this method is responsible for updating a progress bar that is displayed to the user on the terminal.

The problem will be if the FileAdapter class tries to update the amount of bytes downloaded just when the output Thread tries to update the progress bar, as both methods edit the value of the variable intermediateDownloaded, which works only as an auxiliary variable, to hold the amount of bytes downloaded since the last update, to calculate the download speed.

If I use the "synchronized (this)" block, within the downloaded and updateProgress methods, it will block the entire class DownloadReport, and the output Thread will only be able to update the progress bar after the FileAdapter class updates the number of downloaded bytes?

FileAdapter:

public void write(InputStream content, DownloadReport downloadReport) throws IOException {

    FileOutputStream output = new FileOutputStream(file);

    byte[] buffer = new byte[1024];

    int read;

    while ((read = content.read(buffer)) != -1) {
        output.write(buffer, 0, read);
        downloadReport.downloaded(read);
    }
}

DownloadReport:

public void downloaded(int bytes) {
    intermediateDownloaded += bytes;
    downloaded += bytes;
}

public void updateProgress() {

    long now = System.currentTimeMillis();
    double delta = UnitHelper.sizeRound(((now - lastTimeUpdate) / 1000.0), 2);

    if (delta >= 1) {
        unitAdapter.convertSpeed(intermediateDownloaded, delta);

        intermediateDownloaded = 0;
        lastTimeUpdate = now;
    }

    progressBar.updateProgress(unitAdapter.finalSize,
            unitAdapter.recalculate(downloaded), unitAdapter.unity);
}

Answer 1

First of all, this type of synchronization has fallen out of grace in the recent decade or more, because:

• it is extremely hard to do it right
• it suffers performance-wise (due to waiting)
• it is untestable.

Using this approach there may always be bugs in your code, possibly leading to race conditions, and you would not know, and there is no unit test you can write to ensure that your code is free from race conditions. The modern approach to communicating between threads is by passing immutable messages using message queues.

Now, if you insist on doing it this way, synchronize( this ) is a bad idea, because whoever holds a reference to your object can also do synchronized( yourObject ) and then you have a deadlock. The same holds true for synchronized methods, because the compiler implements them using synchronized( this ) under the hood. So, do not do that either. Always declare a private object to use as a lock.

Also, as you seem to already understand, a synchronization lock needs to be active for as little as possible, in order to avoid blocking other threads that might also need to acquire that lock. So, it needs to wrap as few instructions as possible.

In the case of your code, if I understand it correctly, you need to do the following:

private final Object lock = new Object();

public void downloaded( int bytes )
{
    synchronized( lock )
    {
        downloaded += bytes;
    }
}

then further down whenever you access downloaded you must also do it in synchronization with lock, and you need to find some other way of calculating that weird intermediateDownloaded variable of yours based on downloaded only, so that it does not need to take part in the synchronization.

Alternatively, you can replace long downloaded with java.util.concurrent.atomic.AtomicLong downloaded which will allow you to read it and update it much more performantly than if you use synchronization.