Bug #4285

Ruby don't have asynchrounous exception safe syntax and It should have.

Added by Motohiro KOSAKI over 3 years ago. Updated over 1 year ago.

[ruby-core:34537]
Status:Closed
Priority:Normal
Assignee:Koichi Sasada
Category:lib
Target version:next minor
ruby -v:ruby 1.9.3dev (2010-12-22 trunk 30291) [x86_64-linux] Backport:

Description

=begin
This issue was discovered during [Bug#4266] discussion.
Current timeout is racy.

Now, timeout module has following code.


def timeout()
begin
x = Thread.current
y = Thread.start {
begin
sleep sec
rescue => e
x.raise e
else
x.raise exception, "execution expired" if x.alive?
end
}
return yield(sec)
rescue exception => e
raise Error, e.message, e.backtrace
ensure
if y and y.alive?
y.kill
y.join # make sure y is dead.
end
end
end


Unfortunatelly,

y = Thread.start {}

is not an atomic operation. Then, A following race can occur.

CPU0(thread x) CPU1(thread y) remark


enter begin block
[thread construct] but no assign y yet
sleep sec
wakeup from sleep
x.raise
if y return false. (see above)

Therefore, CPU0 don't call y.join and leak y's thread resource. C# have solved
this two-step-construction vs asynchrounous exception race by RAII.
(Plus, C#'s finally block is async exception safe automatically)

But unfortunately, Ruby don't have such language feature. So, We can't write
async-exception-safe code. One of solution is to move timeout module from ruby code
into c code as JRuby does. But I don't think timeout is only asynchrounos exception user.
we also have Interrupt class (for Ctrl-C) and I think we need to allow to write async
exception safe code by ruby.

So, My proposal is,
o for 1.9.x
=> Reimplement timeout.rb by C (as JRuby)

o for 2.0
=> Aim new feature for writing async exception safe code.

Or, Am I missing something?
=end

timeout.rb.diff Magnifier (1.12 KB) Matthias Wächter, 01/18/2011 02:36 AM

timeout.rb.diff Magnifier (1.12 KB) Matthias Wächter, 01/18/2011 02:36 AM


Related issues

Related to ruby-trunk - Bug #4266: Timeouts in threads cause "ThreadError: deadlock; recursi... Closed 01/12/2011
Related to ruby-trunk - Bug #4283: Timeout.timeout may cause application exit unintetionally Closed 01/17/2011
Related to ruby-trunk - Feature #3251: allow to unlock mutex locked by another thread Rejected 05/06/2010
Related to ruby-trunk - Feature #1952: cannot stop with Ctrl+C Closed 08/18/2009
Duplicates ruby-trunk - Feature #6762: Control interrupt timing Closed 07/21/2012

History

#1 Updated by Yusuke Endoh over 3 years ago

=begin
Hi,

2011/1/17 Motohiro KOSAKI redmine@ruby-lang.org:

 CPU0(thread x)                     CPU1(thread y)         remark

 enter begin block
 [thread construct]                                        but no assign y yet
                                    sleep sec
                                    wakeup from sleep
                                    x.raise
 if y                                                      return false. (see above)

Therefore, CPU0 don't call y.join and leak y's thread resource.

What's the resource?
I think that the thread will terminate automatically even if it is
not join'ed.

But unfortunately, Ruby don't have such language feature. So, We can't write
async-exception-safe code. One of solution is to move timeout module from ruby code
into c code as JRuby does. But I don't think timeout is only asynchrounos exception user.
we also have Interrupt class (for Ctrl-C) and I think we need to allow to write async
exception safe code by ruby.

Dr. akr knows a lot about that subject.

--
Yusuke Endoh mame@tsg.ne.jp

=end

#2 Updated by Matthias Wächter over 3 years ago

=begin
Why not use a mutex? Too big a performance hit?

=end

#3 Updated by Matthias Wächter over 3 years ago

=begin
Why not use a mutex? Too big a performance hit?

=end

#4 Updated by Motohiro KOSAKI over 3 years ago

=begin
Endoh-san, Grr, my fault. I thought Thread.join call pthread_join, but it doesn't. So right you are, thread variable leak doesn't cause serious error.
But generically, Asynchrounous exception during OS resource handler class construction is still dangerous. new example is here.

def foo
begin
f = open("example-file")
ensure
f.close
end

timeout (1) {
foo

}

if assignment of f was lost, we have no way to close f.
So, I agree that reimplementation by C doesn't solve anything. because source of problem is not in timeout.rb, is in asynchrounous exception semantics!

So, we don't need anything at 1.9.x timeframe. but I still propose language enhancemnet for any async exception for 2.0.

Mathias, Good question! Your proposal solve almost async execption problem. But please imazine nested timeout (maybe it can occur when
a library use timeout internally) or Ctrl-C. In other words, mutex is effective only when exception source is recognized. But generically, async exception source are not.

=end

#5 Updated by Yusuke Endoh over 3 years ago

=begin
Hi,

2011/1/18 Motohiro KOSAKI redmine@ruby-lang.org:

Endoh-san, Grr, my fault. I thought Thread.join call pthread_join, but it doesn't.

I thought so :-)

But generically, Asynchrounous exception during OS resource handler class construction is still dangerous. new example is here.

def foo
begin
f = open("example-file")
ensure
f.close
end

timeout (1) {
foo
}

if assignment of f was lost, we have no way to close f.

Yes. Surely, the current Thread#raise is dangerous.

I still propose language enhancemnet for any async exception for 2.0.

Basically agreed, but what is needed actually?
I guess that we need these three features, including a semantics
change.

  • a feature to block async exception

    t = Thread.new do
    # exception may be raised
    blockasyncexc do
    # exception may NOT be raised
    end
    # exception may be raised
    end
    t.raise

  • a feature to allow async exception

    t = Thread.new do
    # exception may be raised
    blockasyncexc do
    # exception may NOT be raised
    allowasyncexc do
    # exception may BE raised
    end
    # exception may NOT be raised
    end
    # exception may be raised
    end
    t.raise

  • async exception is blocked by default in rescue clauses and
    ensure clauses

    t = Thread.new do
    begin
    # exception may be raised
    ensure
    # exception may NOT be raised
    allowasyncexc do
    # exception may BE raised
    end
    end
    # exception may be raised
    end
    t.raise

    I remember that we discuss this topic with akr informally, but I forgot
    the conclusion...

    Yusuke Endoh mame@tsg.ne.jp

=end

#6 Updated by Jonas Pfenniger over 3 years ago

=begin
2011/1/18 Yusuke ENDOH mame@tsg.ne.jp:

Basically agreed, but what is needed actually?

I think that ensure blocks should protect us from async exceptions.
They're here to make sure some part of a code is executed, that should
also be the case for async exceptions.

=end

#7 Updated by Jonas Pfenniger over 3 years ago

=begin
Actually, protecting from async exceptions in the ensure block would
be mostly backward-compatible. It means that while a thread is in that
part of the code, no other thread would be running. Only code that
will break, would be code that calls timeout, or does big
calculations, while being in that block.

Another idea: most timeouts are on IO. If IO#gets for example issues
the timeout exception, then it's easy to catch it, you just need to
wrap it with a begin; rescue TimeoutError; end block. It's the same
for the other IOs. Since the timeout happens in C-land, there is no
need to raise that async exception anymore. If that works out well,
then the timeout.rb feature can also be removed. This would require to
set a default timeout on IO and also make it overridable per instance,
but you get the idea.

2011/1/18 Jonas Pfenniger (zimbatm) jonas@pfenniger.name:

2011/1/18 Yusuke ENDOH mame@tsg.ne.jp:

Basically agreed, but what is needed actually?

I think that ensure blocks should protect us from async exceptions.
They're here to make sure some part of a code is executed, that should
also be the case for async exceptions.

=end

#8 Updated by Charles Nutter over 3 years ago

=begin
It is not possible to make cross-thread exception-raising safe with current Ruby semantics.

See the discussion on my blog post here: http://blog.headius.com/2008/02/rubys-threadraise-threadkill-timeoutrb.html

No matter how many locks you use, there's always potential for an arbitrary thread to receive an exception or kill request while executing or immediately before "ensure" code. This breaks the contract of ensure. Ergo, asynchronous exceptions (exceptions thrown from one thread into another) and killable threads are inherently not possible to make safe if you want to obey the contract of "ensure".

JRuby is affected by this as well, since we have to emulate MRI's asynchonous exception throwing features.

There is a way to change Ruby semantics to make asynchronous exceptions and "kill" safe: do not allow them to terminate a thread running an ensure block until that block has completed. However, I think this is mostly useless; a top-level thread that fires logic in its ensure would prevent the entire thread from being killable or raiseable.

Ultimately, both of these features should be removed. And I know that's not going to happen, because everyone uses them :)
=end

#9 Updated by Yusuke Endoh over 3 years ago

=begin
Hi,

2011/1/19 Charles Nutter redmine@ruby-lang.org:

I tried to patch this many times, and it's very difficult. The problem here is that kill is inherently broken in the presence of an ensure block. I've discussed this on ruby-core emails in the past and on my blog here: http://blog.headius.com/2008/02/rubys-threadraise-threadkill-timeoutrb.html

This is not just a problem of Thread#raise. Asynchronous signals
(such as Interrupt caused by Ctrl+C) have the same problem.
Of course, we cannot remove Ctrl+C.

Thus, to address this problem faithfully, we should provide a
mechanism to safely handle asynchronous exceptions. Lobbying to
eliminate only Thread#raise (and #kill) is not facing the reality.

Fortunately, there are some ancient wisdoms:

  • "cancellation points" of pthread
    http://www.kernel.org/doc/man-pages/online/pages/man7/pthreads.7.html

  • Asynchronous Exceptions in Haskell
    http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.26.1040

    These two are based on a very similar idea: providing a feature
    to control whether asynchronous exceptions may be raised or may
    not. In fact, the latter is referred in comments of your blog
    article. But it was rejected as:

    there are a lot of additional problems when implementing it in an environment that isn't as functionally pure as Haskell

    I guess that this is misinterpretation. It is very similar to the
    former (cancellation points), and can be implemented even in
    imperative programming language, as pthread does. I don't know
    that they are compatible with Java (and/or JRuby) threads, though.

    On a separate note, I'm not against deprecating Thread#raise.
    It is indeed too difficult to use correctly. Just eliminating it,
    however, is not enough.

    the child thread may still wake up between the end of the user-defined block and the call to kill

    Yes, it may occur. But does it cause any actual problem in the
    case of timeout.rb? Kosaki's patch seems to me good.

    Yusuke Endoh mame@tsg.ne.jp

=end

#10 Updated by Usaku NAKAMURA about 3 years ago

  • Status changed from Open to Assigned

=begin

=end

#11 Updated by Koichi Sasada almost 2 years ago

  • Description updated (diff)
  • Assignee changed from Yukihiro Matsumoto to Koichi Sasada

#12 Updated by Koichi Sasada over 1 year ago

I don't follow all of discussion about it, this is solved by [ruby-trunk - Feature #6762]?

#13 Updated by Koichi Sasada over 1 year ago

  • Status changed from Assigned to Feedback
  • Target version changed from 2.0.0 to next minor

#14 Updated by Koichi Sasada over 1 year ago

  • Status changed from Feedback to Closed

This issue was solved with changeset r38046.
Koichi, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • thread.c: rename Thread.controlinterrupt to Thread.asyncinterrupt_timing. The option name :never' is also changed to:defer'. [ruby-trunk - Feature #6762]
  • thread.c: remove Thread.check_interrupt. This method is difficult to understand by name.
  • thraed.c: add Thread.async_interrupted?. This method check any defered async interrupts.
  • test/ruby/test_thread.rb: change tests for above.

Also available in: Atom PDF