Bug #8730

"rescue Exception" rescues Timeout::ExitException

Added by Genki Takiuchi almost 2 years ago. Updated over 1 year ago.

[ruby-core:<unknown>]
Status:Rejected
Priority:Normal
Assignee:-
ruby -v:2.0.0 Backport:1.9.3: UNKNOWN, 2.0.0: 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

Duplicated by Ruby trunk - Bug #8845: Timeout::ExitException が意図せずに rescue される事がある Rejected 08/31/2013

Associated revisions

Revision 42690
Added by Nobuyoshi Nakada almost 2 years ago

timeout.rb: skip rescue

  • lib/timeout.rb (Timeout#timeout): should not be caught by rescue clause. [Bug #8730]

Revision 42690
Added by Nobuyoshi Nakada almost 2 years ago

timeout.rb: skip rescue

  • lib/timeout.rb (Timeout#timeout): should not be caught by rescue clause. [Bug #8730]

Revision 43640
Added by Zachary Scott over 1 year ago

Revision 43640
Added by Zachary Scott over 1 year ago

History

#1 Updated by Nobuyoshi Nakada almost 2 years ago

  • Status changed from Open to Rejected

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

#2 Updated by Motohiro KOSAKI almost 2 years ago

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

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

#3 Updated by Nobuyoshi Nakada almost 2 years ago

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

#4 Updated by Motohiro KOSAKI almost 2 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をどう判断した結果なのかとお聞きしました。

#5 Updated by Nobuyoshi Nakada almost 2 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を起こさないようにしていた場合ですが、あるとしたらむしろ意図しないバグだろうと考えました。
前者はまさにその一例ですね。

#6 Updated by Genki Takiuchi almost 2 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 と書いてはならない事になりますが、ここまで来るとやはり
バグであると考えるべきだと思います。

#7 Updated by Yusuke Endoh almost 2 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 mame@tsg.ne.jp

#8 Updated by Motohiro KOSAKI almost 2 years ago

2013/8/28 nobu (Nobuyoshi Nakada) nobu@ruby-lang.org:

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を起こさないようにしていた場合ですが、あるとしたらむしろ意図しないバグだろうと考えました。
前者はまさにその一例ですね。

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

#9 Updated by Nobuyoshi Nakada almost 2 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で丸ごと握りつぶしてしまうのがまずいことはまったく同意ですが、完全ではないにせよそれを避けられるほうがいいのではないでしょうか。

#10 Updated by Motohiro KOSAKI almost 2 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に追記だけしてもらえますか。これで壊れるコードがないかは依然として不明ですが、リスクは低いと思うので
アプリケーション側を直してもらうというのでいいような気がしてきました

#11 Updated by Genki Takiuchi over 1 year ago

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

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

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

#12 Updated by Nobuyoshi Nakada over 1 year ago

そういう場合は

begin
do_something
ensure
log_exception $!
end

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

#13 Updated by Nobuyoshi Nakada over 1 year ago

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

log_exception $! if $!

Also available in: Atom PDF