tfpt review “/shelveset:raise;REDMOND\sborde”
Implements Thread#raise using Thread.Abort, and added tests for it
Implemented stress mode (RubyOptions.UseThreadAbortForSyncRaise) which
found more issues. Fixed most but not all
Enabled test for timeout as well
Remaining work (not high pri for now)
Thread#raise without Exception parameters is not supported as it
needs to access the active exception of the target thread. This is
stored as a thread-local static, and so cannot be accessed from other
threads. Can be fixed by not using ThreadStaticAttribute.
Adding probes (in generated code, in C# library code, etc) will
help to raise the exception quicker as Thread.Abort can be delayed
indefinitely. Ideally, we need both approaches. For now, using
Thread.Abort goes a long way.
Ideally, we would add a try-catch to the IDynamicObject/MetaObject
code paths so that when other languages called into Ruby code, they
would get the expected user exception rather than a ThreadAbortException
RunRSpec: supports -ruby to run with MRI. Its much faster than doing
“rake ruby why_regression”. Added support for -e to run a specific
Shouldn’t the option “UseThreadAbortForSyncRaise” be called
I think that Thread.raise with no arguments should just inject a
RuntimeError with no message as if $! were nil; this makes more sense
than failing. Trying to reference a “current exception” in another
thread is a scary operation even if that’s what MRI is doing.
Other than that, changes look really nice. But anyone thinking of using
this functionality should read Charlie’s excellent piece from earlier in
The terminology I am using throughout is synchronous exception would be
a normal Kernel.raise as the thread knows exactly when and where the
exception will occur. Thread#raise is considered to raise an exception
asynchronously as you cannot control exactly when and where the
exception will actually be raised on the target thread. So with this
terminology, the stress mode should stay as “…ForSyncRaise” so that
Thread.Abort is used even for Kernel.raise.
I will change Thread.raise with no arguments to inject a RuntimeError. I
am not sure if its any better or worse at it depends on whether its
preferable to fail early or to try to keep going. Failing early is
usually a good idea, but in this case, given all the caveats about
Thread#raise, I don’t feel strongly.
kill and raise might be a bit of a bother if you add warnings, since
they’re used in several libraries without any good replacement other
than reimpl (usually to interrupt stuck IO operations). But yeah, I’d be
on board with an across-the-board Thread.critical= warning in both JRuby
Taking a sampling of a few gems, I think it should actually be the other
way. Mongrel and webbrick do use Thread#kill to kill a background
thread, but other gems do not. Thread#raise was also used in just a
couple of places to raise a timeout error. And it is very hard, if not
impossible, to use these APIs safely since you do not know what the
other thread is doing.
OTOH, Thread.critical= can actually be used in a sane way for simple
synchronization, like the lock keyword in C#. This is used way more
widely, and a warning for this will cause lot of false alarms.
I agree that critical= can cause problems, and should go away. However,
it can also be used in a reasonably safe way. Something like this. Such
usages do not rely on other threads being frozen. Most uses seem to use
such a pattern.
Thread.critical = true
cookie = @@counter
@@counter += 1
Thread.critical = false
I am suggesting not adding a warning purely from a pragmatic
perspective. The patter above is used in a lots of of places, and a
warning could irritate users if they see it too often when using their
Is there a way for Ruby devs to control the warning level? If so, it
will be a question of what level to put the warning under, not whether
to add it or not.
-W level sets the warning level. Internally, it sets $VERBOSE to true
if -W, -W1 sets $VERBOSE to false and -W0 sets $VERBOSE to nil. If
$VERBOSE is nil, no warnings (even those called by Kernel.warn) will be
From what I can gather, -v, --version, -w and -W (-W with no level) are
equivalent and all set $VERBOSE to true.
Charlie or Tomas, can you expand on that, or correct me if my
understanding is incorrect?
Is there a way to get the warning printed just the first time it was
executed in the process? Uses like the get_unique_cookie example below
would be called multiple times, and it would be extremely annoying to
print the warning everytime the dangerous API was called.
We could build some internal infrastructure to implement this, but does
Ruby have built-in support we could leverage?
I think the implications of critical= in MRI are more complicated than
just a local lock. It actually stops normal thread scheduling, so other
threads freeze what they’re doing. That’s vastly more intrusive than
just a lock or critical section:
If those other threads are holding locks or resources, deadlock is
extremely easy, and of course it’s nearly impossible to emulate right
with real parallel threads since you can’t easily stop them whenever you
#kill and #raise are also very tricky and dangerous, but they’re at
least localized. They can be defined in terms of a message passed from
one thread to another plus a periodic message checkpoint.
I still believe critical= is much more in need of a warning.
I think I’m coming around to your side. My position is current the
If critical officially remains specified as its current behavior in
MRI, we should be warning people not to use it and should have a warning
emitted at least once.
If we can get ruby-core to agree that critical should be expected to
be no more than a global mutex and not necessarily deschedule threads,
I’d be willing to make the same change in JRuby and omit the warning.
I definitely think we need to talk to ruby-core and open it up to the
community, however, to see if there are cases where people depend on
critical= actually descheduling. Shri: can you propose making the
“global mutex” definition of critical the official one?
-W level sets the warning level. Internally, it sets $VERBOSE to true if -W, -W1 sets $VERBOSE to false and -W0 sets $VERBOSE to nil. If $VERBOSE is nil, no warnings (even those called by Kernel.warn) will be printed.
From what I can gather, -v, --version, -w and -W (-W with no level) are equivalent and all set $VERBOSE to true.
Charlie or Tomas, can you expand on that, or correct me if my understanding is incorrect?
I asked ruby-core whether descheduling other threads is by spec, but no
one other than you replied. Any suggestions on how to nudge others to
consider it? If you want to reply to that thread and provide JRuby’s
vote of confidence for the idea, that might help.
We should nail the complete behavior before taking it to ruby-core. I
have written up a draft at Shri Borde's WebLog | Microsoft Learn.
Please take a look and comment on whether it works for JRuby. We can
move it to some wiki if we need to iterate on it…
Thanks for continuing the thread on ruby-core. Let’s see how it goes.
There are couple of minor issues with Thread.stop releasing the lock:
The code will be hard to read as most people probably don’t realize
that Kernel#stop releases the lock.
Currently, Kernel#sleep does not release the lock. At the least, it
should be consistent with Thread.stop.
If the programmer wants to release the lock before stopping the
thread, its trivial for the programmer to call Thread.critical=false
It’s mostly a question of aesthetics and simplicity. The more
Thread.critical= can be locked down, the more understandable it will be.
But none of these are deal breakers, so I don’t feel too strongly.
About reentrancy, I am proposing that it not be reentrant (unless an
implementation choses to support that behavior). So I think we are on
the same page. Reentrancy would normally have been a good thing, but
since it looks like an assignment, it would read very weirdly in the
Thread.critical=true # if Thread.critical= was reentrant, this would
increment the counter to 2
puts Thread.critical # would be weird if this printed true
Allowing an arbitraty thread to do Thread.critical=false has the same
issue as reentrancy. If Thread.critical stayed true after some thread
did Thread.critical=false, it would be very misleading. Since the thread
cannot actually set Thread.critical to false, it should be an error.
About thread terminating while critical is undefined, some
implementations may be able to know if the dying thread still holds the
lock, but some may not. Especially in the hosted case where some program
is using Ruby as a scripting engine, the hosting app may have created
the thread and run some Ruby snippet on it which entered the global
critical section, but forgot to leave it. In that case, it would be too
burdensome to require that the Ruby implementation deal with this case.
Hence, the proposal of leaving this undefined.