Project

General

Profile

Actions

Bug #8730

closed

"rescue Exception" rescues Timeout::ExitException

Added by takiuchi (Genki Takiuchi) over 10 years ago. Updated over 10 years ago.

Status:
Rejected
Assignee:
-
Target version:
ruby -v:
2.0.0
[ruby-core:<unknown>]

Description

=begin
Timeout.timeout ブロック内で rescue Exception によって例外を捕捉している箇所があると、
タイムアウト処理が内部的に利用している Timeout::ExitException クラスの無名派生クラスを補足してしまい、
正しく Timeout::Error が発生しない。

例)

timeout 1 do
begin
sleep 3
rescue Exception => e
puts e.class.superclass #=> "Timeout::ExitException"
end
end
=end


Related issues 2 (0 open2 closed)

Related to Ruby master - Misc #19740: Block taking methods can't differentiate between a non-local return and a throwClosedActions
Has duplicate Ruby master - Bug #8845: Timeout::ExitException が意図せずに rescue される事があるRejected08/31/2013Actions

Updated by nobu (Nobuyoshi Nakada) over 10 years ago

  • Status changed from Open to Rejected

Exception全体をrescueして握り潰しているのがバグでしょう。

Updated by kosaki (Motohiro KOSAKI) over 10 years ago

ちょっと待った。
現在の挙動は様々な書籍などでも解説されており広く共有されている挙動なので
NEWSも書かずに変えてしまうのは適切じゃないように思います。

そもそもどこで議論した結果?

Updated by nobu (Nobuyoshi Nakada) over 10 years ago

具体的にはどのように解説されてどう利用されているんでしょうか。

Updated by kosaki (Motohiro KOSAKI) over 10 years ago

具体的にはどのように解説されてどう利用されているんでしょうか。

残念ながら紙の書籍は手元にありませんが、

一番多いのは http://kwfsws.g.hatena.ne.jp/kiwofusi/20111231/1325314356 みたいに
rescue Exception => e とすると、意図せずタイムアウトも拾ってしまうので注意しましょうという記述。
これは "ruby timeout 例外" で検索すると似たような注意は何個も出てきます。

次に出てくるのは逆に、デーモンなどで全部捕まえたいケースで、rescue Timeout::Error, StandardError =>e
と書きましょう(書いている)という記述 http://d.hatena.ne.jp/dreammind/20090217/1234813224

2番めは動かなくなりますよね。

それでどこで議論して、Pros/Consをどう判断した結果なのかとお聞きしました。

Updated by nobu (Nobuyoshi Nakada) over 10 years ago

kosaki (Motohiro KOSAKI) wrote:

具体的にはどのように解説されてどう利用されているんでしょうか。

残念ながら紙の書籍は手元にありませんが、

一番多いのは http://kwfsws.g.hatena.ne.jp/kiwofusi/20111231/1325314356 みたいに
rescue Exception => e とすると、意図せずタイムアウトも拾ってしまうので注意しましょうという記述。
これは "ruby timeout 例外" で検索すると似たような注意は何個も出てきます。

なるほど。

次に出てくるのは逆に、デーモンなどで全部捕まえたいケースで、rescue Timeout::Error, StandardError =>e
と書きましょう(書いている)という記述 http://d.hatena.ne.jp/dreammind/20090217/1234813224

2番めは動かなくなりますよね。

Timeout.timeoutから抜けるときには(指定がなければ)Timeout::Errorを投げるので、影響はないですね。

それでどこで議論して、Pros/Consをどう判断した結果なのかとお聞きしました。

意図せずにrescueされてしまう問題点は以前から知られていた問題なので、今回のチケットをきっかけにちょっと試してみたわけです。
非互換性として思いついたのは、timeoutされる処理の中でExceptionをrescueしてtimeoutを起こさないようにしていた場合ですが、あるとしたらむしろ意図しないバグだろうと考えました。
前者はまさにその一例ですね。

Updated by takiuchi (Genki Takiuchi) over 10 years ago

すでに修正されているようなので蛇足気味ですが、

githubなどで公開されているソースコードを調べますと、
rescue Exception を使っているライブラリコードは広く散見され、
書籍などで紹介されている workaround はほとんど浸透していないようです。

この問題の厄介なところは、自分自身が書いたコードだけでなく、利用するライブラリの
コード中にも rescue Exception をしている箇所があると、大外に掛けた timeout が無効化
されてしまう可能性があり、それを予見するのが難しいという点です。

典型的な例としては、以下の様なネットワーク処理を行うライブラリの挙動に
タイムアウトを設定したいというケースがあると思います。

timeout 10 do
  CoolHttpClient.get "http://foo.bar.com"
end

この場合、CoolHttpClient の内部実装のどこかで rescue Exception している箇所が1つでも
あれば、タイミングによっては timeout がもみ消されてしまいます。
他方で、ライブラリ作者としては大外から timeout ブロックで囲われるケースを想定して、
rescue Exception と書いてはならない事になりますが、ここまで来るとやはり
バグであると考えるべきだと思います。

Updated by mame (Yusuke Endoh) over 10 years ago

timeout の変更自体について特に意見はないのですが。

rescue Exception は timeout に限定した問題ではなく、
exit ができないとか、Ctrl-C が効かないとか、トラブルになりがちです。
大した理由もなくそういうことをしているライブラリには
ちゃんと対応してもらうべきだと思います。

また、throw にすれば万事解決というわけではないです。
ensure を使って例外を差し替えることで同じ現象を引き起こせます。

  timeout 1 do
    begin
      begin
        sleep 3
      ensure
        raise
      end
    rescue
    end
  end

--
Yusuke Endoh

Updated by kosaki (Motohiro KOSAKI) over 10 years ago

2013/8/28 nobu (Nobuyoshi Nakada) :

Issue #8730 has been updated by nobu (Nobuyoshi Nakada).

kosaki (Motohiro KOSAKI) wrote:

具体的にはどのように解説されてどう利用されているんでしょうか。

残念ながら紙の書籍は手元にありませんが、

一番多いのは http://kwfsws.g.hatena.ne.jp/kiwofusi/20111231/1325314356 みたいに
rescue Exception => e とすると、意図せずタイムアウトも拾ってしまうので注意しましょうという記述。
これは "ruby timeout 例外" で検索すると似たような注意は何個も出てきます。

なるほど。

次に出てくるのは逆に、デーモンなどで全部捕まえたいケースで、rescue Timeout::Error, StandardError =>e
と書きましょう(書いている)という記述 http://d.hatena.ne.jp/dreammind/20090217/1234813224

2番めは動かなくなりますよね。

Timeout.timeoutから抜けるときには(指定がなければ)Timeout::Errorを投げるので、影響はないですね。

いや、それは blogの誤記だと思いますよ。StandardErrorで捕まえられないエラーがあるからコード変えたという記事なので。

それを最初のメールで補足するべきでしたね。

それに Exception で捕まえてしまうと、SignalExceptionとかも食ってしまうので
一端は捕まえるけど、(ログを吐くなどした後)もう一度raiseするというコードを
書いてない限りは、どの道遅かれ早かれプログラムは誤動作するので、
この修正では問題は解決してないのです。

なので、副作用のほうが気になるわけです。

それでどこで議論して、Pros/Consをどう判断した結果なのかとお聞きしました。

意図せずにrescueされてしまう問題点は以前から知られていた問題なので、今回のチケットをきっかけにちょっと試してみたわけです。
非互換性として思いついたのは、timeoutされる処理の中でExceptionをrescueしてtimeoutを起こさないようにしていた場合ですが、あるとしたらむしろ意図しないバグだろうと考えました。
前者はまさにその一例ですね。

前者がバグであることについては完全に同意します

Updated by nobu (Nobuyoshi Nakada) over 10 years ago

(13/09/05 0:25), KOSAKI Motohiro wrote:

次に出てくるのは逆に、デーモンなどで全部捕まえたいケースで、rescue Timeout::Error, StandardError =>e
と書きましょう(書いている)という記述 http://d.hatena.ne.jp/dreammind/20090217/1234813224

2番めは動かなくなりますよね。

Timeout.timeoutから抜けるときには(指定がなければ)Timeout::Errorを投げるので、影響はないですね。

いや、それは blogの誤記だと思いますよ。StandardErrorで捕まえられないエラーがあるからコード変えたという記事なので。

そのblogにあるコードでは、timeoutが発生するのはnet/httpの中、捕まえるのは外なので、今回の変更による影響はないということです。
StandardErrorで捕まえられるか否かはこれとはまた別件です。

それに Exception で捕まえてしまうと、SignalExceptionとかも食ってしまうので
一端は捕まえるけど、(ログを吐くなどした後)もう一度raiseするというコードを
書いてない限りは、どの道遅かれ早かれプログラムは誤動作するので、
この修正では問題は解決してないのです。

なので、副作用のほうが気になるわけです。

Exceptionで丸ごと握りつぶしてしまうのがまずいことはまったく同意ですが、完全ではないにせよそれを避けられるほうがいいのではないでしょうか。

Updated by kosaki (Motohiro KOSAKI) over 10 years ago

(9/5/13 3:31 AM), Nobuyoshi Nakada wrote:

(13/09/05 0:25), KOSAKI Motohiro wrote:

次に出てくるのは逆に、デーモンなどで全部捕まえたいケースで、rescue Timeout::Error, StandardError =>e
と書きましょう(書いている)という記述 http://d.hatena.ne.jp/dreammind/20090217/1234813224

2番めは動かなくなりますよね。

Timeout.timeoutから抜けるときには(指定がなければ)Timeout::Errorを投げるので、影響はないですね。

いや、それは blogの誤記だと思いますよ。StandardErrorで捕まえられないエラーがあるからコード変えたという記事なので。

そのblogにあるコードでは、timeoutが発生するのはnet/httpの中、捕まえるのは外なので、今回の変更による影響はないということです。
StandardErrorで捕まえられるか否かはこれとはまた別件です。

なるほど

それに Exception で捕まえてしまうと、SignalExceptionとかも食ってしまうので
一端は捕まえるけど、(ログを吐くなどした後)もう一度raiseするというコードを
書いてない限りは、どの道遅かれ早かれプログラムは誤動作するので、
この修正では問題は解決してないのです。

なので、副作用のほうが気になるわけです。

Exceptionで丸ごと握りつぶしてしまうのがまずいことはまったく同意ですが、完全ではないにせよそれを避けられるほうがいいのではないでしょうか。

趣旨はだいたい分かりました。
では NEWSに追記だけしてもらえますか。これで壊れるコードがないかは依然として不明ですが、リスクは低いと思うので
アプリケーション側を直してもらうというのでいいような気がしてきました

Updated by takiuchi (Genki Takiuchi) over 10 years ago

そもそも rescue Exceptionを使うのはまずいという意見が出ていますが、
例えば以下ようにフィルタ的に全てのexceptionを補足してそのままraiseするような
利用をしたい場合があり、実際に使われています。

begin
  do_something
rescue Exception => e
  log_exception e
  raise e
end

libraryコードの中でこのようなコードが一箇所でもあると、大外からtimeoutを掛ける事が
できないので、やはり従前の挙動は問題が多いと思います。

Updated by nobu (Nobuyoshi Nakada) over 10 years ago

そういう場合は

  begin
    do_something
  ensure
    log_exception $!
  end

のほうがいいんじゃないかと。

Updated by nobu (Nobuyoshi Nakada) over 10 years ago

すいません、常にログしてはだめですね。

  log_exception $! if $!
Actions #14

Updated by byroot (Jean Boussier) 9 months ago

  • Related to Misc #19740: Block taking methods can't differentiate between a non-local return and a throw added
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0