How to ensure the correctness of AtomicLong addAndGet result

Question

I want calculate current percentage in my muli-thread download programme.But there is a strange problem . The lastDownloadSize during the second download must be the sum of write and lastDownloadSize of lastDown. example

There is my code

private long getDownloadSize() {
    synchronized (this) {
        final AtomicLong totalWriteCount = new AtomicLong(0);
        final AtomicLong lastDownloadSize = new AtomicLong(0);
        for (DownloadTask task : downloadTasks) {
            final long writeCount = task.getWriteCount();
            totalWriteCount.addAndGet(writeCount);
            final long downloadSize = task.getPosition().getDownloadSize();
            lastDownloadSize.addAndGet(downloadSize);
        }
        System.out.println("=====  writeCount : " + totalWriteCount + "lastDownloadSize : " + lastDownloadSize);
        return totalWriteCount.addAndGet(lastDownloadSize.get());
    }
}

you must define the `AtomicLong` as a *member* of your `class`. Defining it in the method has zero to none usage — Lino, Jun 01 '18 at 06:38

Hoopje · Accepted Answer · 2018-06-01T07:28:43.127

Your totalWriteCount and lastDownloadSize variables are local variables of the getDownloadSize() method. In that case, it does not make sense to use AtomicLong, because only a single thread can access them.

What you probably meant, is to make totalWriteCount and lastDownloadSize members of your class:

class SomeClass {
    // ...
    final AtomicLong totalWriteCount = new AtomicLong(0);
    final AtomicLong lastDownloadSize = new AtomicLong(0);
    // ...

    private long getDownloadSize() {
        synchronized (this) {
            for (DownloadTask task : downloadTasks) {
                final long writeCount = task.getWriteCount();
                totalWriteCount.addAndGet(writeCount);
                final long downloadSize = task.getPosition().getDownloadSize();
                lastDownloadSize.addAndGet(downloadSize);
            }
            System.out.println("=====  writeCount : " + totalWriteCount + "lastDownloadSize : " + lastDownloadSize);
            return totalWriteCount.addAndGet(lastDownloadSize.get());
        }
    }
}

However, also in that case, if they are only accessed from within synchronized(this) blocks, you do not need to use AtomicLongs, because the synchronized block already makes sure that they are only accessed by a single thread at the same time.

Thank you every much .I understand – luckylo Jun 01 '18 at 06:52 — luckylo, Jun 01 '18 at 06:52

score 1 · Answer 2 · answered Jun 01 '18 at 06:46

Your current setup doesn't work, because you're using the AtomicLong the wrong way. Defining any Atomic-class in a single thread is just misusage of that API.

Now why have I said single thread, well you're synchronizing when someone enters your method, which just says that only one thread at a time may use said method. Which leads us to the problem:

The AtomicLong is a local variable

You probably wanted to define your downloadSize and totalWriteCount as a member of your class. E.g:

public class YourClass {
    private final AtomicLong totalWriteCount = new AtomicLong(0);        
    private final AtomicLong downloadSize = new AtomicLong(0);

    /* constructors and other methods */

    private synchronized long getDownloadSize() {
        for (DownloadTask task : downloadTasks) {
            final long writeCount = task.getWriteCount();
            totalWriteCount.addAndGet(writeCount);
            final long downloadSize = task.getPosition().getDownloadSize();
            lastDownloadSize.addAndGet(downloadSize);
        }
        System.out.println("=====  writeCount : " + totalWriteCount + "lastDownloadSize : " + lastDownloadSize);
        return totalWriteCount.addAndGet(lastDownloadSize.get());
    }
}

How to ensure the correctness of AtomicLong addAndGet result

2 Answers2