Project

General

Profile

バグ #746

イベント内で例外が飛んでも握りつぶされてしまうことがある

Added by Osamu Koga over 4 years ago. Updated over 4 years ago.

Status:
終了
Priority:
通常
Target version:
プラグイン名:
ブランチ:
クラッシュする:

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を徹底するというのが理想的かなという気はします。

Associated revisions

Revision 3baf68d2 (diff)
Added by toshi_a 初音 over 4 years ago

イベントコールバック処理中に例外が発生した時、mikutterがクラッシュしない refs #746

Revision 91458f85 (diff)
Added by toshi_a 初音 over 4 years ago

Event#call がselfを返してなかった refs #746

Revision 160a6843 (diff)
Added by Osamu Koga over 4 years ago

Deferredのテストを修正した refs #746

- Deferredのテストがcore/lib/deferred以下に置かれていたので、test/core/libに置き直した。
- mustメソッドを使った形に書き直した。
- ThreadをDeferredとして使った時の挙動が変わったのでテストを修正した。
- 例外は即座にcallerへ伝搬される。
- 通常の例外フローとは独立に、後からtrapやfailすることができる。

History

#1

Updated by toshi_a 初音 over 4 years ago

Deferredを返すのはバグです。Plugin.callが終了したことをフックにして何かをするというのは、そもそもそういったことをサポートすべきかどうか微妙なところだと思います。実装自体は、確かにフィルタの時だけ全く違う実装になっていて、非常に気持ち悪いです。これはどちらかに統一すべきです。

その意味ではDelayerを返していることにも大した意味がなくて、ここはnilやself(callのチェインなんてするかわからないけれど)を返してもいいくらいです。

もちろん例外が潰されるのは問題です。私の意見では、必ずselfを返すようにし、フィルタがあるときにもクラッシュさせるのが良いと思います。イベントが処理された後にDeferred#nextで処理をつなげる需要はなさそうですし、Defarred#failでエラーを検知したいというニーズに至っては、全くないと思います。

プラグインが例外を出したらクラッシュするというのは現在の仕様なので、これをキャッチしたいというのは、別の提案としてチケットを作っていただければと思います。

#2

Updated by toshi_a 初音 over 4 years ago

  • 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の問題も同時に修正してしまっています。一つのコミットになってしまっているのはミスです、ごめんなさい(正月明けから今日まで休みがなくて、2週間前にやりかかってた変更を全て思い出す前にpushしてしまいました)。
変更自体にはだいたい問題はないと思うのですが、今年度中はそんな感じなのでツッコミットもらいたいです。

#3

Updated by Osamu Koga over 4 years ago

確認しました。
Thread#joinを使っているのはevent.rbしかないようなので、問題ないと思います。

で、Deferredのテストが一部失敗するようになっていたので書きなおしていて気付いたんですが、この修正で想定されている挙動は
  • Thread内で例外が発生したら、trapされているかいないかに関わらず、常に例外を上に投げる
  • それとは別にDeferredとしても働くので、後からチェーンをつなぐことも問題なくできる

で合ってますか?
良いようであれば、修正したテストをコミットしておきます。

#4

Updated by Osamu Koga over 4 years ago

  • Status changed from レビュー待ち to 解決
  • Assignee changed from Osamu Koga to toshi_a 初音

しばらく考えてたら、上記の挙動をするのが普通なように思えてきたのでコミットしておきました。

#5

Updated by toshi_a 初音 over 4 years ago

  • Status changed from 解決 to 終了

ありがとうございます

Also available in: Atom PDF