Project

General

Profile

Actions

Bug #4285

closed

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

Added by kosaki (Motohiro KOSAKI) almost 14 years ago. Updated almost 12 years ago.

Status:
Closed
Target version:
ruby -v:
ruby 1.9.3dev (2010-12-22 trunk 30291) [x86_64-linux]
Backport:
[ruby-core:34537]

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


Files

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

Related issues 5 (0 open5 closed)

Related to Ruby master - Bug #4266: Timeouts in threads cause "ThreadError: deadlock; recursive locking"Closedkosaki (Motohiro KOSAKI)01/12/2011Actions
Related to Ruby master - Bug #4283: Timeout.timeout may cause application exit unintetionallyClosedmatz (Yukihiro Matsumoto)01/17/2011Actions
Related to Ruby master - Feature #3251: allow to unlock mutex locked by another threadRejectedko1 (Koichi Sasada)05/06/2010Actions
Related to Ruby master - Feature #1952: cannot stop with Ctrl+CClosedko1 (Koichi Sasada)08/18/2009Actions
Is duplicate of Ruby master - Feature #6762: Control interrupt timingClosedko1 (Koichi Sasada)07/21/2012Actions
Actions #1

Updated by mame (Yusuke Endoh) almost 14 years ago

=begin
Hi,

2011/1/17 Motohiro KOSAKI :

 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

=end

Actions #2

Updated by mwaechter (Matthias Wächter) almost 14 years ago

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

=end

Actions #3

Updated by mwaechter (Matthias Wächter) almost 14 years ago

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

=end

Actions #4

Updated by kosaki (Motohiro KOSAKI) almost 14 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

Actions #5

Updated by mame (Yusuke Endoh) almost 14 years ago

=begin
Hi,

2011/1/18 Motohiro KOSAKI :

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

    block_async_exc 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

    block_async_exc do
    # exception may NOT be raised
    allow_async_exc 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
    allow_async_exc 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

=end

Actions #6

Updated by zimbatm (zimba tm) almost 14 years ago

=begin
2011/1/18 Yusuke ENDOH :

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

Actions #7

Updated by zimbatm (zimba tm) almost 14 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) :

2011/1/18 Yusuke ENDOH :

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

Actions #8

Updated by headius (Charles Nutter) almost 14 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

Actions #9

Updated by mame (Yusuke Endoh) almost 14 years ago

=begin
Hi,

2011/1/19 Charles Nutter :

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:

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

=end

Actions #10

Updated by usa (Usaku NAKAMURA) almost 14 years ago

  • Status changed from Open to Assigned

=begin

=end

Updated by ko1 (Koichi Sasada) over 12 years ago

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

Updated by ko1 (Koichi Sasada) about 12 years ago

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

Updated by ko1 (Koichi Sasada) almost 12 years ago

  • Status changed from Assigned to Feedback
  • Target version changed from 2.0.0 to 2.6
Actions #14

Updated by ko1 (Koichi Sasada) almost 12 years 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.control_interrupt
    to Thread.async_interrupt_timing.
    The option name :never' is also changed to :defer'.
    [ruby-core:50375] [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.
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0