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);
}

stackoverflow.com/help/minimal-reproducible-example

Ravindra Ranwala
– Ravindra Ranwala

2020-03-15 16:40:01 +00:00
Commented Mar 15, 2020 at 16:40 — Ravindra Ranwala
– Ravindra Ranwala, Commented Mar 15, 2020 at 16:40

Mike Nakis · Accepted Answer · 2022-06-29 11:08:41Z

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.

edited Jun 29, 2022 at 11:08

answered Mar 15, 2020 at 16:44

Mike Nakis

63k11 gold badges128 silver badges172 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Rafael F N Xavier Over a year ago

Thank you very much for your help, but I didn't understand why not use "this" inside synchronized, will it block all threads that have an instance of this class?

nits.kk Over a year ago

@Rafael F N Xavier You should try to have a "private" object as lock for synchronised block, Now if u use public property then there may be some code outside of your class which can synchronise on the public property leading to deadlock situations. Same is true with 'this' , there may be code outsode of your class which puts synchronised block on the current object. Now this again can lead to deadlock

Rafael F N Xavier Over a year ago

@nits.kk Thanks, reading your comment and rereading Mike Nakis’ answer, I now understand the problem.

Collectives™ on Stack Overflow

Variable synchronization in multiple Java Threads

1 Answer 1

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related