On Mon, 13 May 2024 21:14:26 GMT, Stuart Marks <[email protected]> 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
