Use safepoint to deliver SIGINT #16174

yuyichao · 2016-05-03T02:36:40Z

This implements @vtjnash 's idea of delivering sigint in a safer way, without having to add sigatomic to everywhere. This also solves a few performance issues along the way. A brief summary of the differences,

Make GC safepoint thread local

And create 3 signal pages. See the comment in safepoint.c for detail.
This have a very minor performance hit for accessing safepoint. However, I think we can avoid the safepoint and GC transition around gcframe setup and gcframe pop on x86 so this shouldn't be a big issue. (On ARM, the atomic release store is more expensive than two GC safepoint so we may use a normal store with two safepoint instead, the performance hit of making the safepoint thread local is still very minor in this case though (~5-10% for empty GC frame push/pop, cheaper than the tls getter call....)).
Use the GC safepoint mechanism (i.e. SegFault) to deliver InterruptException at known points

This automatically makes ccall of external library sigatomic. (Fix Segfault when aborting matrix multiplication #1468, Fix make ccall sigatomic (defer SIGINT handling) #2622). In order to avoid waiting too long before the exception is delivered when running unmanaged code, this also implement waking up libuv, abort syscall and abort libsupport io (this use a hack but can be generalized if needed) when sigint arrives. This also implement force throwing of exception if SIGINT arrives too frequently so that one can use Ctrl-C to force abort some dead loop. A warning will be printed in such case since it bypass the safe path and even sigatomic.

The most important consequence IMHO is that pressing Ctrl-C during sysimg compilation should no longer segfault =)

This should also make it easier to implement signal handling #14675.
Make parser and type inference calls sigatomic

sigatomic is much cheaper (see below) now so it is now used to protect important runtime code to not be interrupted by Ctrl-C.

Clean up safepoint and sigatomic, optimize try-catch and sigatomic

Split out safepoint.c, make defer_signal thread local and avoid the expensive atomic ops
Make defer_signal task local and automatically restore on expection. This eliminate the try-catch needed before in disable_sigint.

This is technically breaking but sigatomic_begin and sigatomic_end aren't exported.
Inline jl_sigatomic_begin and jl_sigatomic_end in codegen.

Benchmarking try-catch and disable_sigint

julia> @inline f1() = try end
f1 (generic function with 1 method)

julia> @inline f2() = disable_sigint(()->nothing)
f2 (generic function with 1 method)

julia> g1(n) = for i in 1:n
           f1()
       end
g1 (generic function with 1 method)

julia> g2(n) = for i in 1:n
           f2()
       end
g2 (generic function with 1 method)

julia> function k(n)
           @time g1(n)
           @time g2(n)
       end
k (generic function with 1 method)

Timing before

julia> k(10_000_000)
  0.264041 seconds
  0.401437 seconds

Timing after

julia> k(10_000_000)
  0.122592 seconds                                                                
  0.012506 seconds

Close #12333
Close #12309

vtjnash · 2016-05-03T03:27:34Z

src/julia.h

+ // that throws a SIGINT.
+ jl_gc_state_save_and_set(eh->gc_state);
+ if (eh->defer_signal == 0) {
+ jl_sigint_safepoint();


i'm fairly certain we don't actually want catch to be a safepoint (now that it's avoidable)

The sigint safepoint is kind of avoidable (by reenable the gc safepoint, edit: however, this would be expensive...) I don't think it's valid to avoid the GC safepoint in the gc state restore though.

that's fine. i just don't think we want to throw an InterruptException before the catch block has a chance to install a try/catch of its own.

We can't really provide the grantee that an async signal will not be thrown between a catch and a try though, unless we garentee it can only be compiled and not interpreted. The GC safepoint can also throw the exception even when there's not a sigint safepoint.

stevengj · 2016-05-03T14:29:49Z

Suppose I have a ccall with a Julia callback function, e.g. ccall(:my_numeric_integration, Cdouble, (Ptr{Void},), cfunction(my_julia_integrand, ...)). If I send a SIGINT while this is executing, does this mean that it will not interrupt the C code, but an InterruptException will still occur during the my_julia_integrand execution?

Also, if you have multiple Tasks running, is there any way to indicate which Task you want to catch the exception? (This is an issue in IJulia.)

yuyichao · 2016-05-03T14:39:30Z

A callback can still throw exception so sigatomic_begin/end are still useful. I made it fast because of this...

The sigint is always delivered to current task on the master thread if it doesn't block sigint (I realized that I might have missed a pending sigint check there, similar to the one in en_restore_state). However, you can block the sigint on the task you don't want it to be delivered by calling sigatomic_begin, is this good enough?

stevengj · 2016-05-03T14:48:25Z

I'm still confused by the callback situation in this PR. If I want InterruptExceptions to be thrown during my callback (e.g. I have a try-catch block that handles them correctly), do I need to call sigatomic_end/begin in my callback?

yuyichao · 2016-05-03T14:48:35Z

For julia callback from C, I think a much faster sigatomic is as far as what this approach can provide.

For the task issue, I'm wondering if we can make non-root tasks sigatomic by default? This way any frontend will need to explicitly reenable sigint on tasks that want to handle sigint.

yuyichao · 2016-05-03T14:51:15Z

(e.g. I have a try-catch block that handles them correctly), do I need to call sigatomic_end/begin in my callback?

A sigint can be delivered in the callback if the C caller wasn't called in sigatomic region. It is a little unreliably though since in principle a sigint can be delivered before the try block starts so I think to do this reliably you need to call the C function with sigatomic and reenable sigatomic in the callback after you started the try block.

stevengj · 2016-05-03T14:52:06Z

I'm not sure if non-root tasks should be sigatomic by default. But it would be nice to have an option to make the task sigint-unaware when it is created. e.g. @async_nosigint

I'm not sure how calling sigatomic_begin/end will help in Tasks, right now. The problem is that sigatomic_end is still called within the Task, and as soon as it is called any deferred SIGINT will be thrown as an exception in that Task. What if I don't want that Task to get InterruptExceptions at all (but I still want them received in other Tasks)?

And what if several tasks are blocking on I/O, and I want a SIGINT to wake up a certain task to catch the n InterruptException?

stevengj · 2016-05-03T14:52:38Z

you need to call the C function with sigatomic and reenable sigatomic in the callback after you started the try block.

Yes, that's what I'm doing now.

yuyichao · 2016-05-03T14:55:33Z

You can call sigatomic_begin without calling end to permanently block sigint on a task. Doing that at initialization time is also possible since I accidentally made defer_signal a property of the task (instead of just save it on the stack on task switch....)

vtjnash · 2016-05-04T16:44:36Z

What is the sigatomic state of a new Task? e.g.

sigatomic_begin()
@async println("am I sigatomic?")
sigatomic_end()
yield()

stevengj · 2016-05-04T17:07:18Z

@yuyichao, sigatomic_begin() will only block SIGINT on the current task? I thought it increments jl_defer_signal, which is global to all tasks?

yuyichao · 2016-05-04T17:14:47Z

sigatomic is now thread and task local. New task currently (on this branch) inherit the state from when it's started... since I haven't set fix start_task for this yet...

yuyichao · 2016-05-05T03:36:46Z

Travis linux32 failure is #15920

tkelman · 2016-05-05T13:49:28Z

src/gc.c

- while (!ptls->gc_state) {
+ // FIXME: The acquire load pairs with the release store
+ // in the signal handler of safepoint so we are sure that
+ // all the memory write on those threads are visible. However,


memory writes? memory written ... is?

Maybe store is the right word?

yuyichao · 2016-05-05T14:41:53Z

Spelling mistakes fixed. Threading CI passed AppVeyor, travis. This should be ready for final review now.

Depending on the caller these functions can be called from unmanaged threads that cannot touch GC state or having GC safepoint.

Add note and function prototypes about using safepoint to deliver SIGINT.

Preparing for using the safepoint to deliver SIGINT, which requires distinguish between master and worker threads.

* Remove unnecessary sigatomic * Make flisp calls sigatomic * Make type inference calls sigatomic * Refactor interthread communication through signal * Make sure `sleep` is aborted on `SIGINT` on Linux to deliver the exception faster * Implement force signal throwing when `SIGINT` arrives too frequently * Hack to abort io syscall on `SIGINT` Fix #1468; Fix #2622; Towards #14675

Also add tests.

stevengj · 2016-05-06T15:28:14Z

It seems like the documentation of the sigatomic functions should be revisited. (I'd also like to see documentation of how to catch ctrl-c in callback functions, disable ctrl-c in certain tasks, etcetera.)

JeffBezanson · 2016-05-06T15:43:16Z

src/signal-handling.c

+ accum_weight = 0;
+ return 0;
+ }
+ double new_weight = accum_weight * exp(-(dt / 1e9)) + 0.3;


Clearly written by a physicist :)

yuyichao · 2016-05-06T15:51:18Z

I've also noticed one case where Ctrl-C can still segfault. It is when we throw the error without any handler, and the execution of on_exit overflows the signal handler stack..... (at least on linux where we throw directly from the signal handler frame). A easy fix would be setting up a top level try catch frame at initialization time. The alternative would be to return to a call to jl_throw from the signal handler instead. However, it seems that setcontext or makecontext using a signal context might be undefined....

JeffBezanson · 2016-05-06T18:24:56Z

src/ast.c

@@ -267,6 +268,7 @@ static jl_ast_context_t *jl_ast_ctx_enter(void)

 static void jl_ast_ctx_leave(jl_ast_context_t *ctx)
 {
+ JL_SIGATOMIC_END();


This breaks --lisp; segfaults.

Ah... ok.... will fix later today....

And in case you need it now, I think making jl_sigint_safepoint noop should work around it for now

Fixed in 417e49a

should be tested if breaking it matters

run(pipeline(DevNull, $(Base.julia_cmd()) --lisp, DevNull)) ?

something like that. might need to be unix only, would need to run it through CI first

tkelman · 2016-05-06T23:00:12Z

This PR broke the Windows buildbots: https://build.julialang.org/builders/package_win6.2-x64/builds/247/steps/make/logs/stdio

Similar to #11727, but now bootstrap freezes when output is redirected instead of just being unable to run tests. @vtjnash @yuyichao please advise, we'll have to revert this if no solution can be found.

yuyichao · 2016-05-06T23:25:57Z

The only commit that touches libuv is the last one and reverting it seems to get rid of the problem for me locally (tested by piping it through tee) I'm checking what is the minimum change that triggers this now...

yuyichao · 2016-05-06T23:48:01Z

Something is fishy here. The buildbot seems to have been printing jl_uv_writecb warnings forever during compilation http:https://buildbot.e.ip.saba.us:8010/builders/package_win6.2-x64/builds/245/steps/make/logs/stdio The freezing happens on current master after printing the first warning. However, either with or without the commit reverted, the build passes without printing any warnings if inference.ji exists before I run make. (i.e. if I run make again after it freezes, the build passes without any warning).

vtjnash · 2016-05-06T23:59:51Z

If you follow that issue, you'll discover it's an upstream bug where libuv forgot to initialize the synchronization object. I think it then corrupts its internal state and tries to report an error that didn't happen by forgetting about a write req that did happen.

yuyichao · 2016-05-07T00:01:14Z

If you follow that issue

Which issue? I didn't see any mentioning of synchronization object in #11727?

vtjnash · 2016-05-07T00:22:17Z

you have to follow it somewhat far. there's a more concise explanation at libuv/libuv#629, in which I demonstrate how it would make nodejs to hang (for the same reason) in a one-liner.

yuyichao · 2016-05-07T00:23:39Z

Ah, that one, I somehow think you are talking about uv_async_t since apparently simply initializing (not even using it) one is causing the trouble on windows......

tkelman · 2016-05-10T01:57:20Z

I think this broke the arm build too: https://build.julialang.org/builders/package_tarballarm/builds/227/steps/make%20binary-dist/logs/stdio

yuyichao · 2016-05-10T02:40:43Z

I believe it's caused by a llvm bug (probably similar to https://llvm.org/bugs/show_bug.cgi?id=27545) easy to workaround by removing an optimization, need to first check if it's fixed on llvm svn...

yuyichao mentioned this pull request May 3, 2016

Add at-sigatomic macro (closes #12309) #12333

Closed

vtjnash reviewed May 3, 2016
View reviewed changes

yuyichao force-pushed the yyc/threads/safepoint-signal branch 2 times, most recently from 1579cfe to 033cfc1 Compare May 4, 2016 16:10

yuyichao force-pushed the yyc/threads/safepoint-signal branch 2 times, most recently from 23088cc to 1b9c1a2 Compare May 4, 2016 23:19

yuyichao force-pushed the yyc/threads/safepoint-signal branch 2 times, most recently from 246654c to d6035ef Compare May 5, 2016 04:36

tkelman reviewed May 5, 2016
View reviewed changes

yuyichao force-pushed the yyc/threads/safepoint-signal branch from d6035ef to 88f7a52 Compare May 5, 2016 14:39

yuyichao added 2 commits May 5, 2016 22:17

Set GC state before calling debuginfo.cpp.

557fa8a

Depending on the caller these functions can be called from unmanaged threads that cannot touch GC state or having GC safepoint.

Add atomic exchange functions

e9c8a7a

yuyichao added 7 commits May 5, 2016 22:17

Enable GC safepoint for non-threading build

893a213

Split out safepoint.c

fcb8fbd

Add note and function prototypes about using safepoint to deliver SIGINT.

Make safepoint thread local.

1022fe2

Preparing for using the safepoint to deliver SIGINT, which requires distinguish between master and worker threads.

Restore delay_sigint on exception and task switch

2ff6098

Also add tests.

Inline jl_sigatomic_begin and jl_sigatomic_end

4337198

Wake up libuv main loop on SIGINT

c589e45

yuyichao force-pushed the yyc/threads/safepoint-signal branch from 88f7a52 to c589e45 Compare May 6, 2016 02:17

vtjnash merged commit b67fa90 into master May 6, 2016

vtjnash deleted the yyc/threads/safepoint-signal branch May 6, 2016 15:05

JeffBezanson reviewed May 6, 2016
View reviewed changes

yuyichao mentioned this pull request May 14, 2016

Better time resolution for cheap functions JuliaCI/BenchmarkTools.jl#7

Closed

This was referenced Jul 5, 2016

Package doesn't work in Julia 0.5 JuliaMath/Cubature.jl#18

Closed

unable to temporarily re-enable interrupts within a Julia callback #17281

Closed

vtjnash mentioned this pull request Jul 20, 2016

AssertionError when hitting ^C at REPL #13664

Closed

yuyichao mentioned this pull request Jul 30, 2016

Unreliable SIGINT delivery #17706

Closed

stevengj mentioned this pull request Sep 23, 2018

Re-enable SIGINT handler in pyjlwrap_call JuliaPy/PyCall.jl#574

Open

rbehrends mentioned this pull request May 6, 2020

Start GAP via Julia oscar-system/GAP.jl#427

Merged

Use safepoint to deliver SIGINT #16174

Use safepoint to deliver SIGINT #16174

Conversation

yuyichao commented May 3, 2016 • edited Loading

Choose a reason for hiding this comment

yuyichao May 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented May 3, 2016 • edited Loading

yuyichao commented May 3, 2016

stevengj commented May 3, 2016

yuyichao commented May 3, 2016

yuyichao commented May 3, 2016

stevengj commented May 3, 2016 • edited Loading

stevengj commented May 3, 2016

yuyichao commented May 3, 2016

vtjnash commented May 4, 2016

stevengj commented May 4, 2016

yuyichao commented May 4, 2016

yuyichao commented May 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuyichao commented May 5, 2016

stevengj commented May 6, 2016

Choose a reason for hiding this comment

yuyichao commented May 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented May 6, 2016

yuyichao commented May 6, 2016 • edited Loading

yuyichao commented May 6, 2016

vtjnash commented May 6, 2016

yuyichao commented May 7, 2016

vtjnash commented May 7, 2016

yuyichao commented May 7, 2016

tkelman commented May 10, 2016

yuyichao commented May 10, 2016

yuyichao commented May 3, 2016 •

edited

Loading

yuyichao May 3, 2016 •

edited

Loading

stevengj commented May 3, 2016 •

edited

Loading

stevengj commented May 3, 2016 •

edited

Loading

yuyichao commented May 6, 2016 •

edited

Loading