Thread#raise, Thread#kill, and timeout.rb are unsafe

I wrote up an article on Thread#raise, Thread#kill, and timeout.rb that 
I hope can start making the rounds. Long story short, neither 
Thread#raise nor Thread#kill is safe to use, and as a result all 
libraries that call them are also unsafe (including timeout.rb, net/*, 
and many other libraries in the wider world).

Have a look, add comments, discuss. Hopefully we can find a way to fix 
this, because it's going to hold Ruby back until we do.

 http://www.**--****.com/ 

- Charlie

Hi,

In message "Re: Thread#raise, Thread#kill, and timeout.rb are unsafe"
    on Mon, 25 Feb 2008 16:18:01 +0900, Charles Oliver Nutter < XXXX@XXXXX.COM > writes:

|I wrote up an article on Thread#raise, Thread#kill, and timeout.rb that 
|I hope can start making the rounds. Long story short, neither 
|Thread#raise nor Thread#kill is safe to use, and as a result all 
|libraries that call them are also unsafe (including timeout.rb, net/*, 
|and many other libraries in the wider world).

Unlike Java, Thread#raise etc. should be treated similar to
KeyboardInterrupt in Ruby.  No realtime exception posting is expected.
If you handle KeyboardInterrupt safely, do same thing for Thread#raise
etc., e.g. just turning on flags to reserve exception, then raise
exception at the safe place, as MRI does.  Nothing more is required.

I admit that timeout.rb is inefficient, so that it should not be used
when performance or completeness matters.  I agree with removing use
of timeout from net/* libraries.  It's just for casual use.  Maybe
it's good to add a note in CAPITAL LETTERS in the document of
timeout.rb.

							matz.

2008/2/25, Charles Oliver Nutter < XXXX@XXXXX.COM >:

There is a typo: your second example misses the "main." before the raise. :-)

I have to think a bit more about this, but one remark: IMHO the
locking should be outside the begin end block.  Reason is, that if
locking fails for a reason you would not want / need to unlock.

At the moment I'm pondering implications of these options:

main = Thread.current
timer = Thread.new { sleep 5; main.raise }
lock(some_resource)
begin
  do_some_work
ensure
  unlock_some_resource
  timer.kill  # moved downwards
end


main = Thread.current
timer = Thread.new { sleep 5; main.raise }
begin
  lock(some_resource)
  begin
    do_some_work
  ensure
    unlock_some_resource
  end
ensure
  timer.kill  # separate ensure
end

The last one has the added benefit that you can implement it in a
method with a block because timer code is not interleaved with work
code.

But both have the disadvantage that you risk getting a timeout
exception during unlock - which would be unfortunate.  OTOH, killing
the timer before the lock release can violate the timing constraint if
the unlocking takes longer - not very likely.

{*filter*} issues; we had some similar issues with changed timeout handling
of a TX manager in JBoss.  Basically the old TX manager was able to
bust an otherwise perfect transaction by timing out in the 2PC
process. :-))

Kind regards

robert

-- 
use.inject do |as, often| as.you_can - without end

I'm not convinced KeyboardInterrupt can be handled safely, either, in a
script.  What happens if a KeyboardInterrupt exception is raised inside
an ensure block?  It could easily result in resource leaks.

I think the right way to handle signals and timeouts is either in an
event loop or a dedicated thread.  Asynchronous exceptions are
convenient but error-prone.

Paul

Hi,

In message "Re: Thread#raise, Thread#kill, and timeout.rb are unsafe"
    on Fri, 14 Mar 2008 06:00:54 +0900, Paul Brannan < XXXX@XXXXX.COM > writes:

|I'm not convinced KeyboardInterrupt can be handled safely, either, in a
|script.  What happens if a KeyboardInterrupt exception is raised inside
|an ensure block?  It could easily result in resource leaks.

You are right, but keyboard interrupt has been there so long in the
history, and we hardly see such leak problems, so I assume it's safer
than you might think.  At least, we cannot not removed the interrupts
from the language.

							matz.

For simple scripts, KeyboardInterrupt is probably the right behavior.

For applications that use an event loop, we can use trap('INT') to
disable KeyboardInterrupt.

I wonder if there should be an equivalent trap mechanism for other
asynchronous exceptions?

Paul

Hi,

In message "Re: Thread#raise, Thread#kill, and timeout.rb are unsafe"
    on Fri, 14 Mar 2008 22:32:33 +0900, Paul Brannan < XXXX@XXXXX.COM > writes:

|I wonder if there should be an equivalent trap mechanism for other
|asynchronous exceptions?

Probably.  Does anyone have any idea?

							matz.

It was the problems with timeout that make me wish we had an 
EnsureCritical (an Ensure that gets Thread.critical=true set to it 
before entering it), so that timeouts can end properly.
Just my $.02
-- 
Posted via  http://www.**--****.com/ 

In article < XXXX@XXXXX.COM >,
  Yukihiro Matsumoto < XXXX@XXXXX.COM > writes:


The problem is, where is safe point.

You said "just turning on flags to reserve exception, then
raise exception at the safe place, as MRI does."

Your "safe" is for interpreter.  Ruby shouldn't SEGV, etc.

But Charles's "safe" is for application.  acquired lock
should be released later, etc.

Your "safe" is not enough for application.

We need a mechanism to delay asynchronous exceptions until a
safe point.

trap is bad way to do it.  trap is similar to Unix signal
handler.  Unix signal handler makes it difficult to avoid
race conditions around blocking operations such as I/O.
With trap, applications need to re-implement the race
condition handling in Ruby level.  It is very difficult if
it is possible.

It is preferable to have a way to declare safe points
directly.

The safe points vary according to applications.  Even in a
application, different kinds of exceptions may have
different safe points.

For example, there are applications KeyboardInterrupt is
safe as you said.  I think a filter, such as grep, is a kind
of the applications.  It means that any points in the
applications are safe points. 

Another example: movemail is a program to move mbox in a
mail spool.  It needs to lock a mbox.  Assume we implement
movemail in Ruby and the lock scheme is file lock.  The lock
file is removed in ensure clause.  But asynchronous
exception in the ensure block may cause to fail removing the
lock file.  So the ensure block is not safe points.

Yet another example: Apache can be stopped with apachectl
"stop" and "graceful-stop".  "stop" aborts open connections
but "graceful-stop" doesn't.  Assume we implement a
threading http server with keep-alive and similar stopping
feature in Ruby.  "stop" can be implemented by killing
threads for each connections by (dangerous) Thread.kill.  It
means that any points in the threads are safe points.
"graceful-stop" need to wait the threads until they waits
requests.  Since keep-alive is assumed, the threads
doesn't terminate after the first request on a connection.
This means the safe points are the request waiting points of
the threads.

I think the safe points declaration mechanism should able to
define safe points for each exception and easy to handle
blocking operations without race conditions.
-- 
Tanaka Akira

Anything using Thread.critical has its own problems, particularly
when native threads are involved.  It can easily result in deadlocks
if the running thread inadvertently tries to use something which another
stopped thread is using (imagine a thread stopped halfway through
releasing a lock that the ensure block needs to acquire, for example
-- something which would have worked fine with normal use of locks, but
becomes a problem once threads can "stop the world").

-mental

The simplest way to "tame" asynchronous exceptions would be to enqueue such
exceptions until the recieving thread makes a blocking call, or else it calls
a special method like Thread.check_interrupts.

The main difficulty with the idea is defining clearly what a "blocking call"
is.  There probably also needs to be some mechanism (which does not "stop
the world" like Thread.critical) to mask asynchronous exceptions even during
blocking calls, for particularly critical code.

-mental

Hi,

In message "Re: Thread#raise, Thread#kill, and timeout.rb are unsafe"
    on Sat, 15 Mar 2008 02:48:33 +0900, Tanaka Akira < XXXX@XXXXX.COM > writes:

|But Charles's "safe" is for application.  acquired lock
|should be released later, etc.

I know.

|Your "safe" is not enough for application.
|
|We need a mechanism to delay asynchronous exceptions until a
|safe point.

Hmm.

|I think the safe points declaration mechanism should able to
|define safe points for each exception and easy to handle
|blocking operations without race conditions.

What should safe point declaration mechanism look like?

							matz.

In a language with side-effects, I've become convinced that the only safe
way to handle asynchronous exceptions is for code to be uninterruptible by
default, and only explicitly allow interrupts at specific points (not broad
regions of code).

This gets back to the fundamental principle that, in order to write a
correct multithreaded program, any interaction between threads needs to be
by explicit mutual agreement at specific places.  One thread needs to
explicitly initiate the interaction (in this case, by calling Thread#raise),
and the other thread needs to explicitly accept it (e.g. by calling a method
which is known to permit the delivery of any pending interrupts when it is
called).  Otherwise, the resulting nondeterminism is impossible to control.

-mental

In article < XXXX@XXXXX.COM >,
  Yukihiro Matsumoto < XXXX@XXXXX.COM > writes:


Basically, asynchronous events should be queued.

* make a queue for each thread.
* Thread#raise(exc) enqueue exc to the queue.
* Thread.check_interrupt(klass) checks an exception which is
  a kind of klass in the queue of the current thread.  If it
  exists, the exception is raised.
* other queue examining mechanism, such as sigpending, etc,
  may be useful.

Thread.check_interrupt is a safe point.

However safe points is not only Thread.check_interrupt.
Blocking operations, such as I/O, may or may not be safe
points.  It is because Thread.check_interrupt with blocking
operations causes race conditions.  So application must
choose that make a blocking operation interruption safe or
uninterruptible.

* Thread.blocking_interruptible(klass) { ... }
* Thread.blocking_uninterruptible(klass) { ... }

Blocking operations in Thread.blocking_interruptible are
safe points.

If a thread has no resources to release, it is safe to kill
asynchronously.  Other than that, safe points should be
declared explicitly.  When a resource is acquired,
application should declare it.

* Thread.delay_interrupt(klass) { ... }

It is safe points that out of outermost
Thread.delay_interrupt.

Another idea is about ensure clause.  Since an ensure
clause is used to release a resource, it may delay
asynchronous events as in Thread.delay_interrupt.
-- 
Tanaka Akira