New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook #19184
Conversation
👋 Welcome back naoto! A progress list of the required criteria for merging this PR into |
@naotoj This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 4 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good.
It's hard to imagine that this shutdown hook might be reading restoreEcho
concurrently with some other thread writing to it. So, our primary goal here is to ensure that whatever writes have been done to restoreEcho
before the hook starts are seen by the hook.
While it's hard to test this change (hence the noreg-hard
label), the restoreEcho
functionality does not seem to be tested at all. Should we add a straightforward test for it, perhaps separately?
Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
Thanks, Pavel.
|
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 |
Right, making the field volatile is a much safer solution. |
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 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 Since we now have established that concurrency is possible, we should find an adequate concurrent solution. I can imagine that the proposed Thread 1 reaches the line marked by 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 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 |
I thought of the same scenario that is certainly possible. Now I am tempted to avoid this race condition altogether by removing checking restoreEcho and always issue echo(true). What do you guys think? |
It should be possible to provide something adequate based on atomic primitives. However, it would complicate code. So, let's first clarify the problem that |
OK, I realized removing checking
|
I think we're assuming here that it's good practice for the Console to snapshot the initial tty state and to restore that state on exit. That's probably right. If we're really supporting that, though, it means that the Console needs to be prepared to deal with whatever state the tty is in. In particular, if echo is off when the JVM starts, then we'll also need to make sure that ordinary reads turn echo on before actually reading. And JLine probably also needs to do things like disable canonical input processing (stty -icanon, or "raw" mode, in Unix parlance). Will these be restored on exit? Hm... I just went and looked a bit through JLine, and it installs its own shutdown hooks! I didn't dig further to see exactly what they do though. Probably they're solving a similar problem though. Unfortunately, shutdown hooks are unordered, so it's possible to that the shutdown hook added at this level will run either before or after JLine's and maybe they'll conflict. So maybe the SPI needs to have tty-save-state and tty-restore-state functions added to it, or something like that, so the responsibility for handling tty stated is pushed down to the provider. But I don't know if JLine could support that. The java.base provider might be able to do something reasonable though. |
Thanks, Stuart.
|
Ah, ok, so each provider has its own shutdown hook, so it's never the case that both hooks are installed at the same time and can conflict with each other. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
898c609 looks robust. Separating fused echo(boolean)
into setEcho(boolean)
and getEcho()
allows us to safely publish final boolean restoreEcho
and use it as many times as we'd like without further synchronisation.
Even though that race between a thread that executes readPassword
and the shutdown hook is still possible, it is now benign: the worst that could happen is that we setEcho(true)
twice. The important thing is that we'll never do a wrong thing.
A general test to show that restoreEcho
works would be good though. Thanks.
OK, I went over all this again, and it looks pretty good, though there's a potential risk. In the old code, the In the new code, the 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? |
Based on an internal discussion regarding Stuart's above comment, I changed the |
I looked at the most recent commit, f69f747. You are right, the race that we hypothesised previously when What we want to do is restore the echo state immediately before JVM exits. We achieve this by installing a shutdown hook which restores the state. However, there's no coordination between the shutdown hook and any thread that also modifies the state. If I read this correctly, due to the mechanics of JVM exit, we simply don't know which thread finishes first: a thread that calls What I'm saying is that we should ensure that the shutdown hook has the final say: once completed, no one should modify echo status. I understand that this means more involved communication between threads. Does it make sense? |
Hi Pavel,
IIUC, the thread that waits on
After the shutdown hooks finish, then the VM is terminated (https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Runtime.html#termination). In there:
So I think the shutdown hook's restoreEcho has the final say, sans the situation if some app installs a shutdown hook with |
I might be confused, but what if the shutdown hook completes and then some application thread enters |
Yes, that is possible. However I would say that it is equally unlikely as an app installs a shutdown hook with readPassword(), so I think this is OK too. BTW, if an app issues |
Okay, but can we call it a best-effort attempt to restore the echo state? I guess, it is a judgement call. |
That would be fair, and exactly what I am aiming for, considering we can do nothing for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest updates look good.
/integrate |
Going to push as commit 236432d.
Your commit was automatically rebased without conflicts. |
Making sure
restoreEcho
correctly reflects the state in the shutdown thread, which differs from the application's thread that issues thereadPassword()
method.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184
$ git checkout pull/19184
Update a local copy of the PR:
$ git checkout pull/19184
$ git pull https://git.openjdk.org/jdk.git pull/19184/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19184
View PR using the GUI difftool:
$ git pr show -t 19184
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19184.diff
Webrev
Link to Webrev Comment