Skip to content

Fix "Fix Ctrl-C Handling in the REPL"#25835

Merged
tgodzik merged 8 commits intoscala:mainfrom
lihaoyi:ctrl-c-part-2
Apr 20, 2026
Merged

Fix "Fix Ctrl-C Handling in the REPL"#25835
tgodzik merged 8 commits intoscala:mainfrom
lihaoyi:ctrl-c-part-2

Conversation

@lihaoyi
Copy link
Copy Markdown
Contributor

@lihaoyi lihaoyi commented Apr 17, 2026

Follow-up from #25782. This PR lets us get the best of both worlds: Ctrl-C handling works even in the subprocess scenario, and System.in.read() works too.

Basically rather than having two threads reading from System.in at the same time, we wrap System.in in our own wrapper such that we can peek at it without dropping the characters we read on the floor. This lets us check for Ctrl-C from the REPL infrastructure while still ensuring that all System.in characters end up being correctly sent to any System.in.read() calls made by user code in the REPL

I haven't looked at the JShell sources since that is prohibited, but it seems they do a similar sort of wrapping, as empirically JShell is able to handle System.in.read while still capturing Ctrl-C and handling that appropriately as well, and System.in in JShell is a wrapper input rather than a raw java.io.BufferedInputStream

jshell> System.in
$1 ==> jdk.jshell.execution.Util$1@31a5c39e

How much have you relied on LLM-based tools in this contribution?

Extensively

How was the solution tested?

Tested manually via ./mill repl.

  1. Verified that without this PR Ctrl-C handling does not work due to the REPL running in a subprocess, with this PR Ctrl-C is properly caught.
  2. Also verified that without this PR, System.in.read() in the REPL returns -1, whereas with this PR System.in.read() in the REPL correctly prompts the user to enter input, and prints the ascii code of the character entered

Also verified the original def askQuestion scenario which now seems to work

@lihaoyi
Copy link
Copy Markdown
Contributor Author

lihaoyi commented Apr 17, 2026

CC @SolalPirelli

if first == -1 then -1
else
bytes(offset) = first.toByte
inputQueue.drainTo(bytes, offset + 1, length - 1) + 1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isnt it possible for state to flip from available to closed between read() and drainTo()? so i guess this method should be synchronized also

Copy link
Copy Markdown
Member

@bishabosha bishabosha Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

answering myself: No because the readUserInputByte() doesnt exit until either the upstream is closed (-1) or it buffered some stuff to inputQueue, in which case drainTo is just clearing what was buffered - and not performing IO.

Copy link
Copy Markdown
Contributor

@SolalPirelli SolalPirelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems correct to me... it's a shame we have to implement a distributed system just to read user input, but having tried everything short of that for my previous PR, I realize it's necessary.

Comment thread repl/src/dotty/tools/repl/JLineTerminal.scala Outdated
Comment thread repl/src/dotty/tools/repl/JLineTerminal.scala Outdated
Comment thread repl/src/dotty/tools/repl/JLineTerminal.scala Outdated
Comment thread repl/src/dotty/tools/repl/JLineTerminal.scala Outdated
Comment thread repl/src/dotty/tools/repl/JLineTerminal.scala Outdated
@lihaoyi
Copy link
Copy Markdown
Contributor Author

lihaoyi commented Apr 17, 2026

I took another pass cleaning up the PR and addressing review feedback

@SethTisue SethTisue added this to the 3.9.0 milestone Apr 17, 2026
@Gedochao Gedochao requested a review from SolalPirelli April 20, 2026 06:23
@tgodzik tgodzik merged commit 7fd3dcd into scala:main Apr 20, 2026
45 checks passed
@tgodzik tgodzik modified the milestones: 3.9.0, 3.8.4 Apr 20, 2026
@tgodzik tgodzik added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Apr 20, 2026
WojciechMazur pushed a commit that referenced this pull request Apr 20, 2026
Follow-up from #25782. This PR lets
us get the best of both worlds: `Ctrl-C` handling works even in the
subprocess scenario, and `System.in.read()` works too.

Basically rather than having two threads reading from `System.in` at the
same time, we wrap `System.in` in our own wrapper such that we can peek
at it without dropping the characters we read on the floor. This lets us
check for Ctrl-C from the REPL infrastructure while still ensuring that
all System.in characters end up being correctly sent to any
`System.in.read()` calls made by user code in the REPL

I haven't looked at the JShell sources since that is prohibited, but it
seems they do a similar sort of wrapping, as empirically JShell is able
to handle `System.in.read` while still capturing Ctrl-C and handling
that appropriately as well, and `System.in` in JShell is a wrapper input
rather than a raw `java.io.BufferedInputStream`

```scala
jshell> System.in
$1 ==> jdk.jshell.execution.Util$1@31a5c39e
```

## How much have you relied on LLM-based tools in this contribution?

Extensively

## How was the solution tested?

Tested manually via `./mill repl`. 

1. Verified that without this PR `Ctrl-C` handling does not work due to
the REPL running in a subprocess, with this PR Ctrl-C is properly
caught.
2. Also verified that without this PR, `System.in.read()` in the REPL
returns `-1`, whereas with this PR `System.in.read()` in the REPL
correctly prompts the user to enter input, and prints the ascii code of
the character entered

Also verified the original `def askQuestion` scenario which now seems to
work
[Cherry-picked 7fd3dcd]
@WojciechMazur WojciechMazur added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Apr 20, 2026
WojciechMazur added a commit that referenced this pull request Apr 20, 2026
Backports #25835 to the 3.8.4-RC2.

PR submitted by the release tooling.
@WojciechMazur WojciechMazur added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Apr 20, 2026
mbovel pushed a commit to mbovel/dotty that referenced this pull request May 4, 2026
Follow-up from scala#25782. This PR lets
us get the best of both worlds: `Ctrl-C` handling works even in the
subprocess scenario, and `System.in.read()` works too.

Basically rather than having two threads reading from `System.in` at the
same time, we wrap `System.in` in our own wrapper such that we can peek
at it without dropping the characters we read on the floor. This lets us
check for Ctrl-C from the REPL infrastructure while still ensuring that
all System.in characters end up being correctly sent to any
`System.in.read()` calls made by user code in the REPL

I haven't looked at the JShell sources since that is prohibited, but it
seems they do a similar sort of wrapping, as empirically JShell is able
to handle `System.in.read` while still capturing Ctrl-C and handling
that appropriately as well, and `System.in` in JShell is a wrapper input
rather than a raw `java.io.BufferedInputStream`

```scala
jshell> System.in
$1 ==> jdk.jshell.execution.Util$1@31a5c39e
```

## How much have you relied on LLM-based tools in this contribution?

Extensively

## How was the solution tested?

Tested manually via `./mill repl`. 

1. Verified that without this PR `Ctrl-C` handling does not work due to
the REPL running in a subprocess, with this PR Ctrl-C is properly
caught.
2. Also verified that without this PR, `System.in.read()` in the REPL
returns `-1`, whereas with this PR `System.in.read()` in the REPL
correctly prompts the user to enter input, and prints the ascii code of
the character entered

Also verified the original `def askQuestion` scenario which now seems to
work
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:done This PR was successfully backported.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants