Possible JDK bug in System.console()

Question

The following code come from java.lang.System.console() method:

private static volatile Console cons = null;

/**
 * Returns the unique {@link java.io.Console Console} object associated
 * with the current Java virtual machine, if any.
 *
 * @return  The system console, if any, otherwise <tt>null</tt>.
 *
 * @since   1.6
 */
 public static Console console() {
     if (cons == null) {
         synchronized (System.class) {
             cons = sun.misc.SharedSecrets.getJavaIOAccess().console();
         }
     }
     return cons;
 }

In my opinion, these is a bug in this method. We should write it like this:

public static Console console() {
     if (cons == null) {
         synchronized (System.class) {
            if (cons == null)
                 cons = sun.misc.SharedSecrets.getJavaIOAccess().console();
         }
     }
     return cons;
 }

Am I right? What's your opinion?

Answer 1

It is not. You can assign a value to it and it already has been initialized. I think the null check is redundant, it is already being checked outside and as Tagir Valeev suggested , inside the JavaIoAccess implementation of console.

You may be thinking about thread contention and because the initialization is happening inside the synchronize block, multiple threads accessing it at the same time will trigger the redundant re-initialization.

So you can say it could be more of an improvement to reduce overhead rather than a bug.

However you should read more on Double checked locking , it contains exactly your scenario. Details below

---

It's interesting to see the subtle problems of using it in J2SE 1.4 (and earlier versions) with various compilers.

After J2SE 5.0 those problem have been fixed

The volatile keyword now ensures that multiple threads handle the singleton instance correctly.

As you will notice the Console static object is volatile.

By using the implementation as described in the above link:

private static volatile Console cons = null;
public static Console console() {
    Console result = console;
    if (result == null) {
        synchronized (System.class) {
            result = console;
            if (result == null) {
                console = result =sun.misc.SharedSecrets.getJavaIOAccess().console();
            }
        }
    }
    return result;
}

You could get such a performance improvement:

Note the local variable result, which seems unnecessary. This ensures that in cases where console is already initialized (i.e., most of the time), the volatile field is only accessed once (due to "return result;" instead of "return console;"), which can improve the method's overall performance by as much as 25 percent - note: replaced helper with console for clarity

But i am not sure how often console() is called from multiple-threads the first time - after that, the initialization is no longer a problem because of the outter null check.

This solution also creates overhead so the actual performance gain is debatable. Thus your suggestion (as adopted above) can be considered at best an improvement.

Answer 2

If you look into the implementation of JavaIOAccess, you will find that there's a null-check inside it:

public Console console() {
    if (istty()) {
        if (cons == null)
            cons = new Console();
            return cons;
        }
    return null;
}

As this method is the only way to initialize that variable, then it's not a problem that the nested null check resides in another method. It's still impossible to get two distinct Console objects.