バグ #746
完了
イベント内で例外が飛んでも握りつぶされてしまうことがある
説明
#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を徹底するというのが理想的かなという気はします。
toshi_a 初音 さんがほぼ10年前に更新
Deferredを返すのはバグです。Plugin.callが終了したことをフックにして何かをするというのは、そもそもそういったことをサポートすべきかどうか微妙なところだと思います。実装自体は、確かにフィルタの時だけ全く違う実装になっていて、非常に気持ち悪いです。これはどちらかに統一すべきです。
その意味ではDelayerを返していることにも大した意味がなくて、ここはnilやself（callのチェインなんてするかわからないけれど）を返してもいいくらいです。
もちろん例外が潰されるのは問題です。私の意見では、必ずselfを返すようにし、フィルタがあるときにもクラッシュさせるのが良いと思います。イベントが処理された後にDeferred#nextで処理をつなげる需要はなさそうですし、Defarred#failでエラーを検知したいというニーズに至っては、全くないと思います。
プラグインが例外を出したらクラッシュするというのは現在の仕様なので、これをキャッチしたいというのは、別の提案としてチケットを作っていただければと思います。
toshi_a 初音 さんがほぼ10年前に更新
- ステータス を 新規 から レビュー待ち に変更
- 担当者 を 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してしまいました）。
変更自体にはだいたい問題はないと思うのですが、今年度中はそんな感じなのでツッコミットもらいたいです。
Osamu Koga さんがほぼ10年前に更新
確認しました。
Thread#joinを使っているのはevent.rbしかないようなので、問題ないと思います。
- Thread内で例外が発生したら、trapされているかいないかに関わらず、常に例外を上に投げる
- それとは別にDeferredとしても働くので、後からチェーンをつなぐことも問題なくできる
で合ってますか？
良いようであれば、修正したテストをコミットしておきます。
Osamu Koga さんがほぼ10年前に更新
- ステータス を レビュー待ち から 解決 に変更
- 担当者 を Osamu Koga から toshi_a 初音 に変更
しばらく考えてたら、上記の挙動をするのが普通なように思えてきたのでコミットしておきました。