2

I'm trying to write a piece of code that allows me to install a library (i.e. download an archive from a remote endpoint and uncompress it on disk) once per JVM (may have several JVMs per machine but it's not the point of the question).

I'm aware of the flaws of the DCL technique regarding memory synchronization, but I'm not sure if I should be concerned by this in my case since my state is on disk.

Can you find any race condition in the following snippet? I couldn't find any information about the thread-safety of filesystem operations in the standard library's documentation (well, some methods are explicitly guaranteed to be atomic but nothing is said about the rest of them).

public class LibraryInstaller {
    // singleton state
    private static AtomicBoolean IS_INSTALLED = new AtomicBoolean();

    // this method will be called hundreds of millions of times. I could remove this requirement but it would force me
    // to use a dirty hack (the justification for that is out of scope)
    public static void ensureInstalled() {
        // double-checked locking to allow faster access after the initial installation
        if (IS_INSTALLED.get()) return;

        synchronized (LibraryInstaller.class) {
            if (!IS_INSTALLED.get()) install();
        }
    }

    // takes several minutes
    private static void install() { 
        downloadLibraryAndUncompressOnDisk();
        IS_INSTALLED.set(true);    
    }
}
Dici
  • 25,226
  • 7
  • 41
  • 82
  • You should make IS_INSTALLED final to ensure safe publication (although it probably safe as it is due to the way static initialization is performed) – Erwin Bolwidt May 31 '18 at 01:15
  • If it was `final` I couldn't it set it to `true` when I do end up installing the library. I could only make it final if the initialization was eager rather than lazy. Or maybe I'm misunderstanding in your comment? – Dici May 31 '18 at 10:09
  • 1
    I am just curious, but why are you not using the [Initialization-on-demand holder idiom](https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom)? – Bhesh Gurung May 31 '18 at 14:55
  • @BheshGurung I didn't know about this idiom. I did think about having it in a static intializer but I didn't think about having a nested class in order to make it lazy. I wonder how this idiom would behave for applications using several classloaders though (which is not my case). I think for my case it's quite a nice fit :) – Dici May 31 '18 at 18:26
  • In case of multiple classloaders, the change in behavior of the idiom would similar to how your current solution would change, IMO. – Bhesh Gurung May 31 '18 at 18:38
  • @BheshGurung yep, I think I can agree with that, at least intuitively! – Dici May 31 '18 at 20:00
  • 1
    The reference being final doesn’t not impact what you can do with the object (AtomicBoolean) that you point to. It only means that you can not replace the reference with a reference to another AtomicBoolean object. Final fields initialized at Construction time have special semantics in the Java Memory Model. – Erwin Bolwidt Jun 01 '18 at 01:14
  • @ErwinBolwidt oh in that sense, ok. I was still in the mindset of using a `volatile` primitive as suggested by @Gregor, not a mutable object. Yeah that's right it could/should be final in that case. – Dici Jun 01 '18 at 11:13

1 Answers1

2

The problem you could run into is that you could potentially block many threads for several minutes, while one thread is inside install(). Not sure what the consequences of that would be in your application. But apart from that your code should work fine and the state of IS_INSTALLED should be visible to all threads just fine.

You should be even able to use a simple private static volatile boolean IS_INSTALLED

Gregor Koukkoullis
  • 2,255
  • 1
  • 21
  • 31
  • While the library isn't present on the path all threads should be blocking indeed, it is an intended behaviour. Fair enough for the `volatile` comment, it's not something I use often but that's right, I'm not using any fancy operation of `AtomicBoolean` in this case so I `volatile` is suitable :p. I'll wait a bit before I approve any answer because for this question it's easier to say there's no problem than to find one! – Dici May 30 '18 at 23:01