Repository navigation
Timeout cannot be called in a trap handler as of 0.3.0 #17
Description
As of Timeout 0.3.0 you can no longer use Timeout in a signal trap handler due to Mutex#synchronize not being callable inside a trap context. I haven't done a git bisect but I believe this was broken in 5e0d8e1 due to the addition of a mutex on @done.
This works fine with 0.2.0.
require "timeout"
rd, wr = IO.pipe
trap("SIGUSR1") do
Timeout.timeout(1) do
end
# Close the pipe writer to unblock the main thread
wr.close
end
# Send USR1 to the current process
Process.kill("USR1", Process.pid)
# Wait for the timeout in the signal handler
rd.read
rd.closeException
./ruby-3.1.2/gems/timeout-0.3.0/lib/timeout.rb:128:in `synchronize': can't be called from trap context (ThreadError)
from ./ruby-3.1.2/gems/timeout-0.3.0/lib/timeout.rb:128:in `ensure_timeout_thread_created'
from ./ruby-3.1.2/gems/timeout-0.3.0/lib/timeout.rb:171:in `timeout'
from test.rb:6:in `block in <main>'
from test.rb:12:in `kill'
from test.rb:12:in `<main>'
Metadata
Metadata
Assignees
Labels
No labels
Activity
hsbt commentedon May 31, 2022
@eregon @headius ?
eregon commentedon May 31, 2022
I don't think there is a way to fix this in this gem.
The Mutex is needed in the code below, to ensure the task is either completed sucessfully or interrupted, but never both:
timeout/lib/timeout.rb
Lines 83 to 97 in 01554f1
An example race without it is we risk in
interruptto read a stale@done(or read it just before it's set byfinished) and could@thread.raiseafter the thread has completed thetimeoutblock, which of course would be a very bad bug.5e0d8e1 just uses it early but wouldn't change anything, it's used anyway in
finished.It could be a simple AtomicBoolean instead of a Mutex (with CAS + read), but there is no such thing in Ruby core or stdlib (only in concurrent-ruby, but somehow AtomicBoolean doesn't seem to have a CAS, only AtomicReference).
There are also 2 other Mutex instances used in Timeout (TIMEOUT_THREAD_MUTEX, QUEUE_MUTEX).
trapin your case?FWIW on CRuby, both Mutex#synchronize and Monitor#synchronize
can't be called from trap context (ThreadError), but Queue#push/pop is allowed (doesn't help us though):gaffneyc commentedon May 31, 2022
We're using
Timeout.timeoutin ourtraphandler to manage graceful shutdown of child processes. I'm sure we could rewrite our code to avoid the issue but it still feels like a regression.eregon commentedon May 31, 2022
It's only a regression on CRuby, so please file an issue to https://bugs.ruby-lang.org/. The change itself (#15) avoids creating one thread per call and has huge performance advantages so we will not revert it just for this.
eregon commentedon May 31, 2022
Already said above, but to be clear there is no need to rewrite the code.
The second way to create a new Thread in
trapshould work fine and require very little changes.Arguably maybe Kernel#trap should create a Thread on its own, much like the JVM does, but that's again something to discuss on the CRuby tracker, not here.
gaffneyc commentedon May 31, 2022
Just to summarize the discussion as I understand it.
The new implementation introduced in #15 uses fewer resources and performs better than the previous version (which is awesome) but requires synchronization to work. There is no way to address the regression because Ruby doesn't have an API for atomic operations (read, write, cas, etc...). A more general solution is to ask the CRuby team to make CRuby act the same way as JRuby.
We'll plan to rework our code to pull the Timeout out of the trap handler and into the main thread instead.
eregon commentedon May 31, 2022
That makes sense, such cleanup should probably be part of
at_exitor an explicitensure.headius commentedon Sep 7, 2022
If you can't synchronize in the trap, perhaps it should just use the old logic that spins up an interrupt thread. Obviously that worked fine before. The new logic is much more efficient, but has introduced this and other issues (#21) so I think we need to provide a multi-faceted solution.
eregon commentedon Mar 2, 2023
CRuby issue: https://bugs.ruby-lang.org/issues/19473
mame commentedon Mar 9, 2023
Because there is no such restriction, there is a low-reproducibility race condition bug in timeout 0.3.2 with JRuby/TruffleRuby. I confirmed it by the following procedure.
sleep 10to timeout as follows (to make it easier to reproduce)CRuby prohibits mutex lock in traps to prevent such a race condition issue.
@eregon Do you think who should address this issue? timeout? Or end users? And how? Cc/ @headius
eregon commentedon Mar 10, 2023
@mame That can be addressed by
return if TIMEOUT_THREAD_MUTEX.owned?beforeTIMEOUT_THREAD_MUTEX.synchronize do(detect reentrant).Or also
Thread.handle_interrupt(Object => :never) doaround theTIMEOUT_THREAD_MUTEX.synchronize do(prevent reentrant).And then AFAIK it works reliably on JRuby and TruffleRuby, but not on CRuby due to that too strict limitation (https://bugs.ruby-lang.org/issues/19473).
A Monitor instead of Mutex here wouldn't fully work because then there could be two threads created (if the trap handlers runs between
unless @timeout_thread and @timeout_thread.alive?and@timeout_thread = create_timeout_thread).We would need a AtomicReference or AtomicBoolean here ideally, but Ruby core provides no such thing. That could be enough on its own in this very specific case, because if two threads get into
ensure_timeout_thread_createdthe second one doesn't need to wait until the timeout thread is created, it just needs to know the thread will be created. It feels brittle though compared to waiting (e.g. if the Thread fails to be created, only one of the two threads will get an exception, that's suboptimal).We could also maybe use
Mutex#try_lockas a workaround for the CRuby too strict limitation, but that means busy-waiting if we want to wait the thread is created. That doesn't seem better, just working around this arbitrary CRuby limitation.eregon commentedon Mar 10, 2023
This doesn't work on CRuby for the example in the description, due to the too strict limitation.
So I believe that proves the CRuby restriction is wrong, because AFAIK the behavior would be fully correct with that:
owned?and let the main thread create the timeout thread (which might happen after the trap handler has returned, that's fine).eregon commentedon Mar 10, 2023
try_lockcould maybe be used forTIMEOUT_THREAD_MUTEXalthough it makes it much harder to reason about and it's incorrect e.g. it will not raise on all threads callingensure_timeout_thread_createdifThread.new{}fails (e.g., no memory or not allowed to create threads, etc).Queue#pop(timeout:), but not on other versions.Request. That used to just usetry_lockand leave the Mutex locked when the Request is done, as a sort of AtomicBoolean, but that doesn't work because CRuby/Ruby releases all Mutex held by a Thread and so that leaks many Mutex until the Thread finishes, and then it takes a long time to release all of them.Maybe we could track the reversed state, i.e. locked initially, unlocked when done, that should avoid this leak/release all problem.
Gracefully handle a call to ensure_timeout_thread_created in a signal…
Gracefully handle a call to ensure_timeout_thread_created in a signal…
[ruby/timeout] Gracefully handle a call to ensure_timeout_thread_crea…
eregon commentedon Jun 24, 2025
I merged that fix in #64.
The situation hasn't changed in that it works fine on TruffleRuby & JRuby, and still doesn't work on CRuby, because CRuby is raising a ThreadError needlessly in this case (https://bugs.ruby-lang.org/issues/19473), as the code in Timeout is 100% correct and valid regarding that
TIMEOUT_THREAD_MUTEXMutex AFAIK.Make Timeout.timeout work in a trap handler on CRuby
eregon commentedon Dec 11, 2025
I found a fix for this, it's not very nice but it works: #81
The idea is on
can't be called from trap context (ThreadError)to instead run the synchronize block in a new Thread and just join that Thread.It's probably not very fast but it only happens on CRuby when inside a trap handler, and to make it faster it would need changes in CRuby to not need to workaround like that, e.g. being able to mark a Mutex as "trap-safe".
Make Timeout.timeout work in a trap handler on CRuby
[ruby/timeout] Make Timeout.timeout work in a trap handler on CRuby