Does overwriting a variable used in a synchronized lock prevent its garbage collection?

Question

I have a cache as a member variable in my service and I'm creating a method to expose it via a JMX MBean, so that I can tear down and recreate my vogon cache at runtime with a new cache expiry time:

public class CachedVogonService implements CachedVogonServiceMBean {

    private LoadingCache<String, Vogon> cache;
    private long expiryInSeconds;
    private VogonService service;

    public CachedVogonService(VogonService newService,
                                long newExpiryInSeconds) {
        this.expiryInSeconds = newExpiryInSeconds;
        this.service = newService;
        this.cache = createCache(newService, newExpiryInSeconds);
    }

    private LoadingCache<String, Vogon> createCache(
            VogonService newService,
            long expiryInSeconds) {
        return CacheBuilder.newBuilder()
                .refreshAfterWrite(expiryInSeconds, TimeUnit.SECONDS)
                .build(new VogonCacheLoader(
                        Executors.newCachedThreadPool(), newService));
    }

    /**
     * This is the method I am exposing in JMX
     */    
    @Override
    public void setExpiryInSeconds(long newExpiryInSeconds) {
        this.expiryInSeconds = newExpiryInSeconds;
        synchronized (this.cache) {
            this.cache = createCache(service, expiryInSeconds);
        }
    }

I'm worried that my locking technique will cause the JVM to keep a reference to the old cache and prevent it being garbage collected.

If my service object loses the reference to the old cache inside the synchronized block, then when execution exits the block, might it then leave the old cache object still marked as locked - making it unavailable for garbage collection?

It definitely shouldn't. If it does that's a bug in the JVM. (Note that replacing a lock like this is considered bad practice, so you might get yelled at for other reasons than garbage collection.) — markspace, Feb 04 '19 at 14:18
are you saying I should create a second variable locally and assign the old cache to that for locking on? — Adam, Feb 04 '19 at 14:21
I'm saying you should use one `final` object for the lock, and never change it. If you change your lock, then any thread could execute that block each with a different lock because you changed the lock. — markspace, Feb 04 '19 at 14:33
OK, got it. I can see i was prematurely trying to minimise the amount of code. — Adam, Feb 04 '19 at 15:10
@Adam, reducing the amount of _source_ code is not always a wise idea. When all other things are equal, the best source code is the code that is most _readable_, and your code will be more readable if you don't do any tricky things that other developers are not accustomed to see. Ideally, when you write `synchronized(foo)`, other developers would like `foo` to be the name of a `final` variable because that is one sure way to prevent a common newbie mistake in which `foo` does not always refer to the object that the developer _thought_ it would refer to. — Solomon Slow, Feb 04 '19 at 16:18
to clarify, this isn't just about readability or vaguely bad practices. trying to swap out the object locked on undoes the whole locking scheme and lets threads into the protected block concurrently. this is BROKEN. variables aren't locked on, objects are. — Nathan Hughes, Feb 04 '19 at 21:37
The best source code is the code that does the job correctly at minimum cost. There are many, many costs: non-readability is only one of them. @SolomonSlow — user207421, Feb 05 '19 at 00:56

miskender · Accepted Answer · 2019-02-09T11:33:01.413

By looking at the byte-code that is generated for similar case, We can see object address of locking field is duplicated and used to acquire and release of the lock. So original locking object is used for locking.

After You change locking field with new object inside synchronized block, another thread can acquire lock on the new object, and can enter the synchronized code block. Using synchronized like this, does not provide synchronization between threads. You should use another object for locking. (like final Object cacheLock = new Object())

Just for information purposes, this kind of usage does not prevent garbage collection. Since object addresses mentioned here are inside the stack frame of this method, once the method finishes execution, stack frame will be destroyed and no reference to old object will remain. (But don't use synchronized like this.)

You can check JVM Instruction Set here

public class SyncTest {

    private Long value;

    public static void main(String[] args) {
        new SyncTest().testLock();
    }

    public SyncTest() {
        value = new Long(1);
    }

    private void testLock() {
        synchronized (this.value) {
            this.value = new Long(15);
        }
    }
}

  private void testLock();
     0  aload_0 [this]
     1  getfield t1.SyncTest.value : java.lang.Long [27] // push value field to operand stack
     4  dup // duplicate value field push it to operand stack
     5  astore_1 // store the top of the operand stack at [1] of local variable array (which is the duplicated value field)
     6  monitorenter // aquire lock on top of the operand stack 
     7  aload_0 [this]
     8  new java.lang.Long [22]
    11  dup
    12  ldc2_w <Long 15> [31]
    15  invokespecial java.lang.Long(long) [24]
    18  putfield t1.SyncTest.value : java.lang.Long [27]
    21  aload_1 // load the [1] of local variable array to the operand stack (which is the duplicated value field previously stored)
    22  monitorexit // release the lock on top of the operand stack
    23  goto 29
    26  aload_1
    27  monitorexit
    .....

Does overwriting a variable used in a synchronized lock prevent its garbage collection?

1 Answers1