inconsistent synchronisation violation

Question

I got this violation on return statement in following method:

protected Token getAccessToken() {  
    synchronized (this) {
        if (token == null || isExpired(token))
            token = createToken();
    }

    return token; // <-- Inconsistent synchronization of blablabla.token; locked 75% of time
}

Are there any visibility issues related to token field? As I understand after synchronized block token should have its latest value.

Am I missing something or it is false positive?

Answer 1

Consider the following:

• Thread1: Enters method
• Thread2: Enters method
• Thread1: Enters sync block, token not null and not expired
• Thread1: Exits sync block
• Thread2: Enters sync block, token not null but expired
• Thread2: Assigns new token
• Thread1: Returns token (might be new value assigned by thread 2, might be old value)
• Thread2: Exits sync block
• Thread2: Returns (new) token

If you want to do what you are doing, then token might need to be volatile (but this might not be a sufficient guarantee!), or you should always return the value from the synchronized block, or assign the value of token to a local variable inside the synchronized block and return that local variable from outside.

This doesn't even consider what other methods might be doing to token in the meantime. If another (synchronized or unsynchronized) method modifies token as well (eg assigns null), then you might be in worse shape because you assume that token is not null (as you just checked that), while in reality it might be null now.

Answer 2

Thread A might return token that has just been recreated by thread B because the token was expired.

So thread B will write token from a synchronized block, but thread B will read it from an unsynchronized block. So yes, there might be problems. The return should be inside the synchronized block.