-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: more reliable & extensible ^C REPL interrupt #14032
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,61 +37,115 @@ abstract AbstractREPL | |
answer_color(::AbstractREPL) = "" | ||
|
||
type REPLBackend | ||
repl_channel::Channel | ||
response_channel::Channel | ||
in_eval::Bool | ||
ans | ||
backend_task::Task | ||
REPLBackend(repl_channel, response_channel, in_eval, ans) = | ||
new(repl_channel, response_channel, in_eval, ans) | ||
repl_channel::Channel # channel for AST | ||
response_channel::Channel # channel for results: (value, nothing) or (error, backtrace) | ||
task::Task # current backend task | ||
responded::Bool # flag indicating the state of this backend | ||
end | ||
|
||
function put_backend_response(backend::REPLBackend, response, validsource::Bool) | ||
if validsource && !backend.responded | ||
backend.responded = true | ||
put!(backend.response_channel, response) | ||
else | ||
@schedule begin | ||
println(STDERR, "Got a REPL eval result, but already returned a result:") | ||
value, bt = response | ||
if bt !== nothing | ||
showerror(STDERR, value, bt) | ||
else | ||
show(STDERR, value) | ||
end | ||
end | ||
end | ||
nothing | ||
end | ||
|
||
# note: the name of this function is significant for printing error backtraces | ||
function eval_user_input(ast::ANY, backend::REPLBackend) | ||
iserr, lasterr, bt = false, (), nothing | ||
while true | ||
iserr, lasterr = false, (nothing, ()) | ||
while !backend.responded | ||
try | ||
if iserr | ||
put!(backend.response_channel, (lasterr, bt)) | ||
put_backend_response(backend, lasterr, backend.task == current_task()) | ||
iserr, lasterr = false, () | ||
else | ||
ans = backend.ans | ||
# note: value wrapped in a non-syntax value to avoid evaluating | ||
# possibly-invalid syntax (issue #6763). | ||
eval(Main, :(ans = $(getindex)($(Any[ans]), 1))) | ||
backend.in_eval = true | ||
value = eval(Main, ast) | ||
backend.in_eval = false | ||
backend.ans = value | ||
put!(backend.response_channel, (value, nothing)) | ||
eval(Main, :(ans = $(Expr(:quote, value)))) | ||
put_backend_response(backend, (value, nothing), backend.task == current_task()) | ||
end | ||
break | ||
catch err | ||
if iserr | ||
println("SYSTEM ERROR: Failed to report error to REPL frontend") | ||
println(err) | ||
end | ||
iserr, lasterr = true, err | ||
bt = catch_backtrace() | ||
iserr, lasterr = true, (err, catch_backtrace()) | ||
end | ||
end | ||
end | ||
|
||
""" set_unhandled_exception_handler(f) | ||
Register the function `f` as the current unhandled exception handler. | ||
If `f` is not a function, the handler is uninstalled. | ||
The return value is the current exception handler, or `nothing` if none is installed. | ||
""" | ||
function set_unhandled_exception_handler(f) | ||
ccall(:jl_set_unhandled_exception_handler, Any, (Any,), f) | ||
end | ||
|
||
function start_repl_backend(repl_channel::Channel, response_channel::Channel) | ||
backend = REPLBackend(repl_channel, response_channel, false, nothing) | ||
backend.backend_task = @schedule begin | ||
local backend # keep track of the current backend | ||
# each backend can look at this to see if it has been replaced | ||
# by the unhandled_exception_handler | ||
new_backend_task = function() | ||
# include looks at this to determine the relative include path | ||
# nothing means cwd | ||
while true | ||
while current_task() == backend.task | ||
tls = task_local_storage() | ||
tls[:SOURCE_PATH] = nothing | ||
ast, show_value = take!(backend.repl_channel) | ||
ast, show_value = take!(repl_channel) | ||
if show_value == -1 | ||
# exit flag | ||
# graceful exit flag | ||
set_unhandled_exception_handler(last) | ||
break | ||
end | ||
backend.responded = false | ||
eval_user_input(ast, backend) | ||
end | ||
nothing | ||
end | ||
backend = REPLBackend(repl_channel, response_channel, Task(new_backend_task), true) | ||
schedule(backend.task) | ||
|
||
last = set_unhandled_exception_handler() do excpt | ||
if istaskdone(backend.task) | ||
# our backend is gone, so reset to the previous exception handler | ||
set_unhandled_exception_handler(last) | ||
last === nothing || last() | ||
return | ||
end | ||
|
||
# setup a handler for unhandled exceptions (such as from root task or finish_task_hook) | ||
# that tries to forward these errors to the REPL frontend | ||
if isa(excpt, InterruptException) && backend.task.state == :runnable && isempty(Base.Workqueue) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the purpose of checking for an empty workqueue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. conservatism in minimizing the change in the conditional from the old version. i think the idea was to allow the backend task to try to unstick itself unless that seemed unlikely to happen on its own |
||
# attempt to forward the interrupt to the backend task | ||
Base.throwto(backend.task, excpt) | ||
else | ||
# otherwise, try to return this failure directly to the REPL frontend | ||
# and orphan the old backend | ||
if !backend.responded | ||
backend.task = Task(new_backend_task) # let the old backend keep spinning (albeit without a consumer now) | ||
end | ||
put_backend_response(backend, (excpt, catch_backtrace()), true) | ||
schedule(backend.task) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: this needs to be conditional on backend.task being new |
||
end | ||
wait() | ||
# returning from this function would signal to the system that the error couldn't be handled | ||
end | ||
|
||
backend | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1011,6 +1011,7 @@ export | |
consume, | ||
current_task, | ||
istaskdone, | ||
istaskstarted, | ||
lock, | ||
notify, | ||
produce, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -240,6 +240,7 @@ static void NOINLINE NORETURN start_task(void) | |
// this runs the first time we switch to a task | ||
jl_task_t *t = jl_current_task; | ||
jl_value_t *res; | ||
t->started = 1; | ||
if (t->exception != NULL && t->exception != jl_nothing) { | ||
record_backtrace(); | ||
res = t->exception; | ||
|
@@ -800,6 +801,16 @@ DLLEXPORT void gdbbacktrace(void) | |
jlbacktrace(); | ||
} | ||
|
||
jl_function_t *jl_unhandled_exception_handler; | ||
DLLEXPORT jl_value_t *jl_set_unhandled_exception_handler(jl_value_t* v) | ||
{ | ||
jl_value_t *old = (jl_value_t*)jl_unhandled_exception_handler; | ||
if (jl_is_function(v)) | ||
jl_unhandled_exception_handler = (jl_function_t*)v; | ||
else | ||
jl_unhandled_exception_handler = NULL; | ||
return old ? old : jl_nothing; | ||
} | ||
|
||
// yield to exception handler | ||
void NORETURN throw_internal(jl_value_t *e) | ||
|
@@ -810,11 +821,24 @@ void NORETURN throw_internal(jl_value_t *e) | |
jl_longjmp(jl_current_task->eh->eh_ctx, 1); | ||
} | ||
else { | ||
jl_printf(JL_STDERR, "fatal: error thrown and no exception handler available.\n"); | ||
if (jl_unhandled_exception_handler) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: i believe there needs to be a assignment |
||
JL_TRY { | ||
jl_apply(jl_unhandled_exception_handler, &e, 1); | ||
} | ||
JL_CATCH { | ||
jl_printf(JL_STDERR, "fatal: unhandled_exception_handler threw an error.\n"); | ||
jl_static_show(JL_STDERR, jl_exception_in_transit); | ||
jl_printf(JL_STDERR, "\nwhile handling:\n"); | ||
} | ||
} | ||
else { | ||
jl_printf(JL_STDERR, "fatal: error thrown and no exception handler available.\n"); | ||
} | ||
jl_static_show(JL_STDERR, e); | ||
jl_printf(JL_STDERR, "\n"); | ||
jlbacktrace(); | ||
jl_exit(1); | ||
abort(); | ||
} | ||
assert(0); | ||
} | ||
|
@@ -863,6 +887,7 @@ DLLEXPORT jl_task_t *jl_new_task(jl_function_t *start, size_t ssize) | |
t->gcstack = NULL; | ||
t->stkbuf = NULL; | ||
t->tid = 0; | ||
t->started = 0; | ||
|
||
#ifdef COPY_STACKS | ||
t->bufsz = 0; | ||
|
@@ -952,6 +977,7 @@ void jl_init_root_task(void *stack, size_t ssize) | |
jl_current_task->ssize = ssize; | ||
jl_current_task->stkbuf = stack; | ||
#endif | ||
jl_current_task->started = 1; | ||
jl_current_task->parent = jl_current_task; | ||
jl_current_task->current_module = jl_current_module; | ||
jl_current_task->tls = jl_nothing; | ||
|
@@ -972,6 +998,11 @@ void jl_init_root_task(void *stack, size_t ssize) | |
jl_task_arg_in_transit = (jl_value_t*)jl_nothing; | ||
} | ||
|
||
DLLEXPORT int jl_is_task_started(jl_task_t *t) | ||
{ | ||
return t->started; | ||
} | ||
|
||
#ifdef __cplusplus | ||
} | ||
#endif |
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.
Did you verify that #6763 doesn't crop back up with this change?
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.
i hadn't, but trying it now it seems to work as expected. it looks like i need to delete the above comment too.
i was modifying this on the implementation of the similar function in client.jl:
07a0c09
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.
the changes to this function fix #13955