14

I recently found this gem in my code base:

/** This class is used to "publish" changes to a non-volatile variable.
 *
 * Access to non-volatile and volatile variables cannot be reordered,
 * so if you make changes to a non-volatile variable before calling publish,
 * they are guaranteed to be visible to a thread which calls syncChanges
 *
 */
private static class Publisher {
    //This variable may not look like it's doing anything, but it really is.
    //See the documentaion for this class.
    private volatile AtomicInteger sync = new AtomicInteger(0);

    void publish() {
        sync.incrementAndGet();
    }

    /**
     *
     * @return the return value of this function has no meaning.
     * You should not make *any* assumptions about it.
     */
    int syncChanges() {
        return sync.get();
    }
}

This is used as such:

Thread 1

float[][] matrix;
matrix[x][y] = n;
publisher.publish();

Thread 2

publisher.syncChanges();
myVar = matrix[x][y];

Thread 1 is a background updating thread that runs continuously. Thread 2 is a HTTP worker thread that does not care that what it reads is in any way consistent or atomic, only that the writes "eventually" get there and are not lost as offerings to the concurrency gods.

Now, this triggers all my warning bells. Custom concurrency algorithm written deep inside of unrelated code.

Unfortunately, fixing the code is not trivial. The Java support for concurrent primitive matrices is not good. It looks like the clearest way to fix this is using a ReadWriteLock, but that would probably have negative performance implications. Correctness is more important, clearly, but it seems like I should prove that this is not correct before just ripping it out of a performance sensitive area.

According to the java.util.concurrent documentation, the following create happens-before relationships:

Each action in a thread happens-before every action in that thread that comes later in the program's order.

A write to a volatile field happens-before every subsequent read of that same field. Writes and reads of volatile fields have similar memory consistency effects as entering and exiting monitors, but do not entail mutual exclusion locking.

So it sounds like:

  • matrix write happens-before publish() (rule 1)
  • publish() happens-before syncChanges() (rule 2)
  • syncChanges() happens-before matrix read (rule 1)

So the code indeed has established a happens-before chain for the matrix.

But I'm not convinced. Concurrency is hard, and I'm not a domain expert. What have I missed? Is this indeed safe?

Improve this question
1
  • 1
    Shouldn't publisher.sync(); in Thread 2 be publisher.syncChanges()? Commented Feb 5, 2013 at 19:45

4 Answers 4

Reset to default
4

In terms of visibility, all you need is volatile write-read, on any volatile field. This would work

final    float[][] matrix  = ...;
volatile float[][] matrixV = matrix;

Thread 1

matrix[x][y] = n;
matrixV = matrix; // volatile write

Thread 2

float[][] m = matrixV;  // volatile read
myVar = m[x][y];

or simply
myVar = matrixV[x][y];

But this is only good for updating one variable. If writer threads are writing multiple variables and the read thread is reading them, the reader may see an inconsistent picture. Usually it's dealt with by a read-write lock. Copy-on-write might be suitable for some use patterns.

Doug Lea has a new "StampedLock" http://gee.cs.oswego.edu/dl/jsr166/dist/jsr166edocs/jsr166e/StampedLock.html for Java8, which is a version of read-write lock that's much cheaper for read locks. But it is much harder to use too. Basically the reader gets the current version, then go ahead and read a bunch of variables, then check the version again; if the version hasn't changed, there was no concurrent writes during the read session.

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

Comments

4

This does look safe for publishing single updates to the matrix, but of course it doesn't provide any atomicity. Whether that's okay depends on your application, but it should probably be documented in a utility class like this.

However, it contains some redundancy and could be improved by making the sync field final. The volatile access of this field is the first of two memory barriers; by contract, calling incrementAndGet() has the same effect on memory as a write and a read on a volatile variable, and calling get() has the same effect as a read.

So, the code can rely on the synchronization provided by these methods alone, and make the field itself final.

Improve this answer

6 Comments

Even for single updates to the matrix, this Publisher class is doing nothing at all. There is no guarantee of the other thread seeing the updates after it calls synchChanges(), because synchChanges() does not check if the data has been published.
@RussellZahniser It's not "doing nothing at all;" it is ensuring that updates are visible across threads. I understand that it doesn't provide any means for the reader to predicate its actions on receipt of an update. But, if all you are doing in the other thread is displaying the "current" state, perhaps periodically on a timer, it might not be necessary to wait on such a condition. While it doesn't wait for a message to be received, this mechanism does ensure that a message is sent. Without a memory barrier, you can't even be assured of that.
But it doesn't actually "ensure that updates are visible across threads." The reader may or may not see the updates. To get the intended effect, you would need something like a volatile boolean isPublished which the writer sets true only when it is done writing and will never write again, and the reader takes action only when it sees true.
Being visible and being seen are different. Without the Publisher, updates aren't visible. Without testing for receipt, actions may occur before an update is seen. Whether this distinction is important depends on the application. To say that the publisher does nothing, however, is false.
By "does nothing" I mean that whether the reader calls synch() or not, it may see some, all, or none of the changes made by the writer. Updates may be visible without calling synch(), and they may also be visible if you call synch(), but the call has no effect on this.
|
2

Using volatile is not a magic bullet to synch everything. It is guaranteed that if another thread reads the updated value of a volatile variable, they will also see every change made to a non-volatile-variable before that. But nothing guarantees that the other thread will read the updated value.

In the example code, if you make several writes to matrix and then call publish(), and the other thread calls synch() and then reads the matrix, then the other thread may see some, all, or none of the changes:

  • All the changes, if it reads the updated value from publish()
  • None of the changes, if it reads the old published value and none of the changes have leaked through
  • Some of the changes, if it reads the previously published value, but some of the changes have leaked through

See this article

Improve this answer

2 Comments

Isn't the other thread guaranteed to read the updated value of a volatile variable because of the volatility? That's what volatile is... From your article: "The Java volatile modifier is an example of a special mechanism to guarantee that communication happens between threads. When one thread writes to a volatile variable, and another thread sees that write, the first thread is telling the second about all of the contents of memory up until it performed the write to that volatile variable."
Ah, I see what you mean. I do not care that the reader may come in before the writer. The writer is in an infinite loop on a background thread and the reader is a HTTP worker in response to a request.
1

You are correctly mentioned the rule #2 of happens-before relationship

A write to a volatile field happens-before every subsequent read of that same field.

However, it doesn't guarantee that publish() will ever be called before syncChanges() on the absolute timeline. Lets change your example a bit.

Thread 1:

matrix[0][0] = 42.0f;
Thread.sleep(1000*1000); // assume the thread was preempted here
publisher.publish(); //assume initial state of sync is 0

Thread 2:

int a = publisher.syncChanges();
float b = matrix[0][0];

What are the options for a and b variables are available ?

  • a is 0, b can be 0 or 42
  • a is 1, b is 42 because of the happens-before relationship
  • a is greater than 1 (Thread 2 was slow for some reason and Thread 1 was lucky to publish updates several times), value of b depends on the business logic and the way matrix is handled - does it depend on the previous state or not?

How to deal with it? It depends on the business logic.

  • If Thread 2 polls the state of a matrix from time to time and it's perfectly fine to have some outdated values in between, if in the end the correct value will be processed, then leave it as is.
  • If Thread 2 doesn't care about missed updates but it always wants to observe up-to-date matrix then use copy-on-write collections or use ReaderWriteLock as it was mentioned above.
  • If Thread 2 does care about single updates then it should be handled in a smarter way, you might want to consider wait() / notify() pattern and notify Thread 2 whenever matrix is updated.
Improve this answer

1 Comment

It is entirely OK for the read to happen before any processing happens at all. I am not trying to emulate a latch here. Writes happen continuously in the background, and reads come from a HTTP worker thread pool.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.