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

[Stuart Marks]

On Mon, 20 May 2024 17:49:29 GMT, Naoto Sato <naoto at openjdk.org> wrote:

>> Making sure `restoreEcho` correctly reflects the state in the shutdown thread, which differs from the application's thread that issues the `readPassword()` method.
>
> Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
>  - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
>  - copyright year
>  - Merge branch 'master' into JDK-8332084-restoreEcho-readLock
>  - get/setEcho()
>  - Addresses a review comment
>  - Replaced another unused exception with _
>  - Update src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java
>    
>    Co-authored-by: ExE Boss <3889017+ExE-Boss at users.noreply.github.com>
>  - initial commit

OK, I went over all this again, and it looks pretty good, though there's a potential risk.

In the old code, the `restoreEcho` variable was usually false, and it was only true while `readPassword` was running, which then set it back to false. The shutdown hook would therefore almost always see a value of false and decide to do nothing.

In the new code, the `restoreEcho` variable stores the initial state of the echo status, which is usually true. We don't keep track of whether readPassword() has set the echo status, since that leads to mutable state and race conditions etc. as discussed previously. The only time the shutdown hook doesn't set the echo status is if the initial echo status of the terminal is false, which probably almost never occurs. Thus, the shutdown hook almost always sets the echo status to true regardless of whether it's necessary to do so.

I'm not concerned about the amount of work the shutdown hook is doing. I'm wondering if there's a possibility that setting the terminal modes almost every time on exit is the right thing. It might block, potentially hanging the VM shutdown. ... Reading further, the case I was concerned about was if somebody runs a Java program from a shell and then puts it into the background. Then the JVM shuts down and the shutdown hook tries to enable echo. Does this work, or does it cause the process to be stopped with SIGTTIN or something? And if it works, would it restore echo immediately, even if say a foreground process were reading a password?

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

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