CODEWITHSUNDEEP
javamultithreadingthread-safetysingletonlocking
Sameer
Sameer

Reputation: 807

Doubly Check Lock Dosen't work on this java code?

I am trying to fix a service by optimising the synchronisation blocks. I am getting two different values and my doubly check singleton with a volatile string dosen't seem to work?

The get and Increment string puts a row lock on DB and increments the string so the unique update is taken care at DB level only. So in the first case, no issues.

Problem lies in the else blozk. When correlation ID is not null, then we try and fetch an already mapped value, if this is the first call. then we first map the value and then return it. This mapping has to be synchronised so that two different threads don't update next val agter both of them find it to be null.

This class is also a singleton service.

public class RangeQueryService{

private volatile String nextValue=null;


public String getNextIncrement(String name, String correlationId) throws SomeCheckedException {
    try {
            if (correlationId == null) {

                nextValue = rangeFetch.getAndIncrementAsString(name);

            } else { //Enter Sync branch

                // mapper Will Return null if no value is mapped.

                nextValue = mapper.mapToB(SOME_CONST, correlationId);

                // Avoid syncronization overhead if value is already fetched. Only enter if nextVal is null.

                if (nextValue == null) {

                    synchronized (this) {
                        Doubly Check lock pattern, as two threads can find null simultaneously, and wait on the critical section.

                        if(nextValue==null){
                            nextValue = rangeFetch.getAndIncrementAsString(name);
                            idMapper.mapToB(SOME_CONST, correlationId, nextValue, DURATION);
                        }
                    }
                }
            }

        return nextValue;
    } catch (Exception e) {
        throw new SomeCheckedException("Error!" + e.getMessage());
    }
}

It Return both 19 and 20. it should only return 19.

Output:

headerAfterProcessOne: 0000000019, headerAfterProcessTwo: 0000000020

Upvotes: 1

Views: 65

Answers (1)

Steyrix
Steyrix

Reputation: 3236

If I understood you in the right way, you expect one thread (lets call it A) to wait for another (which increments the value to 19, B), and then skip the incrementing because nextValue is 19 and not null. But the change is unseen by the waiting thread.

The possible scenario is much more complicated, as I see:

The problem is that 19 returned by the A thread, the one which waits, as it immediately skips the whole block after the volatile value is posted by the B thread at line:

nextValue = rangeFetch.getAndIncrementAsString(name);

Another case, the A thread enters the method and nextValue is already posted.

So it immediately jumps to return statement and return 19, which was set by the B thread (Yes, it is unexpected, but yet this happens sometimes). You should not expect the A thread to wait for the B to finish executing. The B (which reached synchronized block first) finish processing (incrementing) the value and returns 20.

There are possible workarounds for that, though:

if (nextValue == null) {
    synchronized(this) {
        if(nextValue == null) {
            String localTemp = rangeFetch.getAndIncrementAsString(name);
            idMapper.mapToB(SOME_CONST, correlationId, localTemp, DURATION);
            nextValue = localTemp;
        }
    }
}

The overall point is that changes made to nextValue immediately affect another calls of getNextIncrement.

It is really hard to debug problems in this pattern, so I may be wrong, but I post an answer anyway, since my explanation is too long for the comment.

Upvotes: 0

Related Questions