RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v3]

[Pavel Rappo]

On Mon, 13 May 2024 21:14:26 GMT, Stuart Marks <smarks at openjdk.org> wrote:

> Hm, I don't think we want to add any synchronized blocks within a shutdown hook. If a thread is blocked reading from the console, it will hold readLock; if the JVM is then shut down using a signal, it will run shutdown hooks, and this hook will block awaiting readLock. This can deadlock the shutdown completely.
> 
> To ensure proper memory visibility, maybe consider making `restoreEcho` be volatile.

It turns out it's not that hard to imagine. Thanks for helping me imagine it, Stuart.

I think that we should add the most straightforward test to demonstrate that the `restoreEcho` logic (or the lack of it) has an observable effect. If we fail to do that, it might mean that the `restoreEcho` logic is not needed and that the rest of this message is probably irrelevant.

The test should be added not in a separate PR for [8332161] -- a bug which Naoto has just filed -- but in this PR, and then we should `/issue add 8332161`. 

Since we now have established that [concurrency] is possible, we should find an adequate concurrent solution. I can imagine that the proposed `volatile`-based solution may fall short in the following scenario.

Thread 1 reaches the line marked by `*`. Let's assume that immediately before executing that line `restoreEcho` is `false` and echo is on:


public char[] readPassword(String fmt, Object ... args) {
    char[] passwd = null;
    synchronized (writeLock) {
        synchronized(readLock) {
            installShutdownHook();
            try {
                restoreEcho = echo(false); // *
            } catch (IOException x) {
                throw new IOError(x);
            }
            IOError ioe = null;
            try {
                if (!fmt.isEmpty())
                    pw.format(fmt, args);
                passwd = readline(true);
            } catch (IOException x) {
                ioe = new IOError(x);
            } finally {
                try {
                    if (restoreEcho) // **
                        restoreEcho = echo(true);
                } catch (IOException x) {
            ...


Thread 1 executes `echo(false)`, which returns `true`. `true` is not yet written into `restoreEcho`.

Meanwhile, thread 2 (the hook thread) is about to execute the line marked by `***`:


public void run() {
    try {
        if (restoreEcho) { // ***
            echo(true);
        }
    } catch (IOException x) { }
}


Thread 2 sees `restoreEcho` is `false` and skips `echo(true)`. If thread 1 then ceases to exist (remember, we're shutting down) before it reaches the line marked by `**`in the `finally` block, the console echo will remain off.

[8332161]: https://bugs.openjdk.org/browse/JDK-8332161
[concurrency]: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Runtime.html#shutdown

-------------

PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2109779741