Skip to content
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

CTRL-C does not quit repl when in infinte loop #6612

Closed
SamSaffron opened this issue May 6, 2016 · 15 comments · Fixed by #6635
Closed

CTRL-C does not quit repl when in infinte loop #6612

SamSaffron opened this issue May 6, 2016 · 15 comments · Fixed by #6635
Assignees
Labels
repl Issues and PRs related to the REPL subsystem.

Comments

@SamSaffron
Copy link

SamSaffron commented May 6, 2016

Platform in Linux x64

Node version 6.0.0

repro:

% node
> while(true){}

CTRL-C does nothing

Since the process handles SIGINT fine when sent via kill I think this is due to ->Run in v8 somehow blocking all keyboard input.

@Fishrock123 Fishrock123 added the repl Issues and PRs related to the REPL subsystem. label May 6, 2016
@Fishrock123
Copy link
Contributor

I'm not sure this is actually possible to fix due to how the repl works.

@addaleax
Copy link
Member

addaleax commented May 6, 2016

I’d like to try and take this one if nobody minds.

@ghost
Copy link

ghost commented May 6, 2016

I'm pretty sure that there is no way to get back to the repl after an infinite loop. Just CTRL+C twice will quite back to the terminal.

@ghost
Copy link

ghost commented May 6, 2016

As well as this there almost certainly isn't a fix

@bnoordhuis
Copy link
Member

bnoordhuis commented May 6, 2016

@addaleax You're probably going to have to take a similar approach to what I did in #5904: start a watchdog thread, wake it from the SIGINT signal handler, and (presumably) make it call isolate->TerminateExecution().

I suspect you'll need to insert a C++ stack frame between the REPL and the evaluated code, with a TryCatch try_catch that calls isolate->CancelTerminateExecution() when try_catch.HasTerminated() is true.

If you figure out how to fix the semaphore issue on FreeBSD, let me know. :-)

EDIT: That's for Unices. I don't know how you'd approach this on Windows.

@yorkie
Copy link
Contributor

yorkie commented May 6, 2016

How about just making Node.js REPL to be a pure proxy/manager to another node ChildProcess, to resolve this issue, just kill that ChildProcess instance, is that simple enough? correct me if I'm wrong :-)

@addaleax
Copy link
Member

addaleax commented May 6, 2016

@bnoordhuis Yep, that sounds about right.

I’d add an option to the vm.runIn(This)Context functions for catching the signal… that doesn’t seem too hard, there’s already a lot in place to support the timeout argument and a lot of the timeout Watchdog class seems reusable.

I don’t have any local access to FreeBSD machines, though… are the CI runs from that PR still available somewhere? And would anything speak against uv_async_send for waking up the thread? That uses only write() under the hood, as far as I can tell.

@addaleax
Copy link
Member

addaleax commented May 6, 2016

@yorkie You mean, having a “front” REPL interface that proxies to another process which does what the current REPL does? That may be possible, but it would probably be a lot more intrusive. Any state would be lost in the case of killing it, so it’s not that big an improvement over the current situation… or am I misunderstanding something?

@yorkie
Copy link
Contributor

yorkie commented May 6, 2016

@addaleax thanks for your comment, here is my respond :-)

having a “front” REPL interface that proxies to another process which does what the current REPL does?

Yea, exactly.

That may be possible, but it would probably be a lot more intrusive.

Could you please expand what do you mean by intrusive?

Any state would be lost in the case of killing it

Yea, when we externally kill that child process, the state would be get lost. But what's the real case?

@bnoordhuis
Copy link
Member

are the CI runs from that PR still available somewhere?

Probably not. I think they're kept around for 30 days. After that, they're deleted.

And would anything speak against uv_async_send for waking up the thread?

Instead of a semaphore? uv_async_send() doesn't claim to be async signal-safe; it should work in practice but that's more of an implementation detail than a guarantee.

@addaleax
Copy link
Member

addaleax commented May 6, 2016

@yorkie

Could you please expand what do you mean by intrusive?

Sure, sorry for being unclear. I just think it would probably be more work to get the default eval of the REPL to use a child process for communication than it would be to catch a signal – I may be wrong about that, of course.

But what's the real case?

Not sure what you mean – the ideal way to resolve Ctrl+C would be to stop execution of the current command and return to the REPL, and still have everything from before it in place (unless, of course, the interrupted command modified something).

@addaleax
Copy link
Member

addaleax commented May 6, 2016

@bnoordhuis

Instead of a semaphore? uv_async_send() doesn't claim to be async signal-safe; it should work in practice but that's more of an implementation detail than a guarantee.

Yes… but if I understood you correctly, uv_sem_post suffers from the reverse problem?

@yorkie
Copy link
Contributor

yorkie commented May 6, 2016

the ideal way to resolve Ctrl+C would be to stop execution of the current command and return to the REPL, and still have everything from before it in place (unless, of course, the interrupted command modified something).

You are right, just an advice :-)

@bnoordhuis
Copy link
Member

if I understood you correctly, uv_sem_post suffers from the reverse problem?

I looked into it just now. uv_sem_post() in #5904 seems to be working properly, it fixes the failing test.

Another test, parallel/test-debug-signal-cluster, is now failing consistently (but only on freebsd) but that test has been extraordinarily flaky in the past; I'm rather distrustful of it.

addaleax added a commit to addaleax/node that referenced this issue Jun 18, 2016
Adds the ability to stop execution of the current REPL command
when receiving SIGINT. This applies only to the default eval
function.

Fixes: nodejs#6612
PR-URL: nodejs#6635
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@SamSaffron
Copy link
Author

wow, thank you!

Fishrock123 pushed a commit that referenced this issue Jun 27, 2016
Adds the ability to stop execution of the current REPL command
when receiving SIGINT. This applies only to the default eval
function.

Fixes: #6612
PR-URL: #6635
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit that referenced this issue Jul 5, 2016
Adds the ability to stop execution of the current REPL command
when receiving SIGINT. This applies only to the default eval
function.

Fixes: #6612
PR-URL: #6635
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants