1

Given the following code:

public class SomeClass {

  private boolean shouldBlock = false;
  private Object resource;

  public void handleDrawRequest(Canvas canvas) {
    if (!shouldBlock && resource == null)
    {
       shouldBlock = true;
       loadTheResource();  //which takes awhile
       shouldBlock = false;
    }
    else if (shouldBlock && resrouce == null)
    {
       return;  //another thread is taking care of the loading of the resource
                //and its not ready yet, so just ignore this request
    }

    drawResourceOn(canvas);
  }
}

How can I make this code thread safe? What I'm trying to accomplish is for one and only one thread to load the resource while any other thread attempting access this code at the same time to be discarded (e.g. follow the 'else if' logic) until the resource is loaded. There could be many threads trying to access this code at the same time and I don't want to synchronize the entire method and have a whole stack of threads pile up.

Chris Knight
  • 24,333
  • 24
  • 88
  • 134

2 Answers2

4

With double checked non-blocking locking:

public class SomeClass {

    private Lock lock = new Lock();
    private volatile Object resource;

    public void handleDrawRequest(Canvas canvas) {
        if( resource==null ) {
            if( lock.tryLock() ) {
                try {
                    if( resource==null )
                        resource = loadResource();
                }
                finally {
                    lock.unlock();
                }
            }
            else {
                return;
            }
        }
        drawResourceOn(canvas);
    }
}

If you don't make resource volatile, threads are free to cache it and might never read the updated value. In particular, the second null check will always return true, even if the resource has been loaded after the first one.

Piotr Praszmo
  • 17,928
  • 1
  • 57
  • 65
  • Nice one, I've created the same solution in Eclipse and then noticed that answer is posted :-) – Petro Semeniuk Feb 12 '12 at 03:06
  • @Banthar looks really good thanks. Can you explain the need to make the resource volatile? – Chris Knight Feb 12 '12 at 08:21
  • 1
    @ChrisKnight It's to ensure a happens-before relationship between one thread calling `resource = loadResource()` and another thread seeing a non-null `resource`. Without `volatile`, the second thread could see a non-null but partially-constructed `resource` Object. – yshavit Feb 12 '12 at 10:16
2

You're looking for an AtomicBoolean

public class SomeClass {
  // AtomicBolean defaults to the value false.
  private AtomicBoolean loadingResource = new AtomicBoolean();
  private volatile Object resource;

  public void handleDrawRequest(Canvas canvas) {
    if (resource == null) {
      if (loadingResource.compareAndSet(false, true)) {
        loadTheResource();  //which takes awhile
      } else {
        //another thread is taking care of the loading of the resource
        //and its not ready yet, so just ignore this request
        return;  
      }
    } else {
      drawResourceOn(canvas);
    }
  }
}
OldCurmudgeon
  • 64,482
  • 16
  • 119
  • 213
  • Not sure if you need the resource `volatile`. – OldCurmudgeon Feb 12 '12 at 01:51
  • looks nice and simple. However, loading the resource could fail, leaving resource null and I'd like future threads to be able to retry loading the resource. Can AtomicReference help here? Or is it as simple as adding loadingResource.compareAndSet(true, false) after loadTheResource()? – Chris Knight Feb 12 '12 at 08:31
  • @Chris, if loading the resource fails, the AtomicBoolean can be reset to false. – Scorpion Feb 12 '12 at 10:32
  • Works a treat, even if 'compareAndSet' is gramatically clunky. Answer awarded for simplicity of solution. For the record, in `loadTheResource()` I've added a finally block which checks for the resource being null (i.e. loading failed) and then executed this code within: `loadingResource.set(false);` – Chris Knight Feb 13 '12 at 22:54