バグ #746
closed
イベント内で例外が飛んでも握りつぶされてしまうことがある
Added by Osamu Koga almost 10 years ago.
Updated over 9 years ago.
Description
#745 を追っていて気付いたんですが、以下の条件を全て満たす時に例外が握りつぶされ、どこにも報告されない（Activityやnoticeに出ない、例外で落ちない）状態になります。
- イベントが1つ以上のフィルタをもつ
- Plugin.callによって発火されたフック(on_xxx)内で例外が発生する
- Plugin.callがterminate（ないしtrap）されていない
たとえば、on_updateがバグっていて例外を投げるようなプラグインがいた場合、notify_observersが途中で失敗して、そのプラグイン以降のon_updateが実行されなくなりますが、エラーログは全く残らないのでデバッグがとても難しくなります。
event.rb#53-59 のあたりで、例外が起きたらpromise.failに包んで上に持って行こうとしているのが直接的な原因です（他のcallフローではそういう処理をしていないのも怪しい）。
バグが起きた時に追うのが面倒ですし、例外の意義からしてもログが全く残らないというのは良くないので直したほうが良さそうです。
そもそもcallのRDocにはDelayerを返すと書いてあるので、Deferredが返ってくるようになっているのがおかしいように思います。
しかし、個人的にはプラグインの投げた例外もちゃんとcatchしてくれるような機構が欲しいと思っているので、callしたら常にDeferredが返ってくるようにしてしまい、Plugin.callしたらterminateを徹底するというのが理想的かなという気はします。
Deferredを返すのはバグです。Plugin.callが終了したことをフックにして何かをするというのは、そもそもそういったことをサポートすべきかどうか微妙なところだと思います。実装自体は、確かにフィルタの時だけ全く違う実装になっていて、非常に気持ち悪いです。これはどちらかに統一すべきです。
その意味ではDelayerを返していることにも大した意味がなくて、ここはnilやself（callのチェインなんてするかわからないけれど）を返してもいいくらいです。
もちろん例外が潰されるのは問題です。私の意見では、必ずselfを返すようにし、フィルタがあるときにもクラッシュさせるのが良いと思います。イベントが処理された後にDeferred#nextで処理をつなげる需要はなさそうですし、Defarred#failでエラーを検知したいというニーズに至っては、全くないと思います。
プラグインが例外を出したらクラッシュするというのは現在の仕様なので、これをキャッチしたいというのは、別の提案としてチケットを作っていただければと思います。
- Status changed from 新規 to レビュー待ち
- Assignee set to Osamu Koga
修正している最中に、DeferredがThreadで発生する例外を全てトラップしてしまい、mikutter.rb で Thread.abort_on_exception が true になっているにも関わらずクラッシュしないという問題を見つけました。当然この挙動が放置されてきたということは abort_on_exception な挙動をされることは想定していないということで、mikutter内ではThreadの例外をキャッチするために Deferredable#trap を使ってきました。Threadの例外は全て握りつぶしていましたが、こちらでは例外を受け取ることが出来ていました。
本件を解決するにあたって、上記の挙動を是正しました。イベントの実行順序を発火された順番にするために、Deferredではなく Thread#value を使っていますが、これだと abort_on_exception がtrueなのでインタプリタが終了してしまいテストが出来ませんでした。またDeferredがThreadの例外を握りつぶすのも同時に問題になってきます。
そういうわけで、Threadの問題も同時に修正してしまっています。一つのコミットになってしまっているのはミスです、ごめんなさい（正月明けから今日まで休みがなくて、２週間前にやりかかってた変更を全て思い出す前にpushしてしまいました）。
変更自体にはだいたい問題はないと思うのですが、今年度中はそんな感じなのでツッコミットもらいたいです。
確認しました。
Thread#joinを使っているのはevent.rbしかないようなので、問題ないと思います。
で、Deferredのテストが一部失敗するようになっていたので書きなおしていて気付いたんですが、この修正で想定されている挙動は
- Thread内で例外が発生したら、trapされているかいないかに関わらず、常に例外を上に投げる
- それとは別にDeferredとしても働くので、後からチェーンをつなぐことも問題なくできる
で合ってますか？
良いようであれば、修正したテストをコミットしておきます。
- Status changed from レビュー待ち to 解決
- Assignee changed from Osamu Koga to toshi_a 初音
しばらく考えてたら、上記の挙動をするのが普通なように思えてきたのでコミットしておきました。
- Status changed from 解決 to 終了
Also available in: Atom
PDF
Updated by 
Updated by