最適化 #1047
完了
deprecated な User#to_i が使われている
説明
Retriever::Model::Memory もしくは Diva::Model::Memory で Retriever::Model::User の to_i メソッドが使われている
あひる 家鴨 さんがほぼ7年前に更新
- ステータス を 新規 から パッチ適用待ち に変更
- 担当者 を あひる 家鴨 から toshi_a 初音 に変更
- プラグイン名 を core/ lib/diva_hacks/model/memory.rb から core/ lib/retriever/model/memory.rb に変更
とりあえず hotfix/3.5 向けに変更をコミットしました
3.6-develop だと当該変更箇所は core/ lib/diva_hacks/model/memory.rb にあり、 3.5 の core/ lib/retriever/model/memory.rb とは違います。（retriever -> diva_hacks なだけではありますが）
この場合、どちらのブランチに向けてコミットすればよろしいでしょうか。
toshi_a 初音 さんがほぼ7年前に更新
- ステータス を パッチ適用待ち から レビュー待ち に変更
- 担当者 を toshi_a 初音 から あひる 家鴨 に変更
hotfix/3.5 にmergeしました。一応確認お願いします。
toshi_a 初音 さんがほぼ7年前に更新
- ステータス を 終了 から 実装待ち に変更
- 担当者 を あひる 家鴨 から toshi_a 初音 に変更
d4a3463d を適用した状態でPostBoxからリプライを投稿しようとすると、APIリクエスト開始前にクラッシュします。
リリース前に気づけたので、このチケットで問題の切り分けを行います。
/home/toshi/Projects/mikutter/core/service.rb:152:in `scan': The method can not calls main thread. but called by main thread. (ThreadError)
from /home/toshi/Projects/mikutter/core/message.rb:697:in `findbyid'
from /home/toshi/Projects/mikutter/core/lib/retriever/model/identity.rb:22:in `findbyid'
from /home/toshi/Projects/mikutter/core/lib/retriever/model/identity.rb:26:in `generate'
from /home/toshi/Projects/mikutter/core/lib/mikutwitter/api_shortcuts.rb:125:in `update'
from /home/toshi/Projects/mikutter/core/service.rb:183:in `call'
from /home/toshi/Projects/mikutter/core/service.rb:183:in `block in define_postal'
from /home/toshi/Projects/mikutter/core/service.rb:199:in `block (2 levels) in define_postal'
from /home/toshi/Projects/mikutter/core/service.rb:205:in `block in <class:Service>'
from /home/toshi/Projects/mikutter/core/service.rb:198:in `block in define_postal'
from /home/toshi/Projects/mikutter/core/message.rb:111:in `post'
from /home/toshi/Projects/mikutter/core/mui/gtk_postbox.rb:151:in `post_it'
from /home/toshi/Projects/mikutter/core/plugin/gtk/gtk.rb:337:in `block (2 levels) in <top (required)>'
from /home/toshi/Projects/mikutter/vendor/bundle/ruby/2.4.0/gems/pluggaloid-1.1.1/lib/pluggaloid/listener.rb:25:in `call'
from /home/toshi/Projects/mikutter/vendor/bundle/ruby/2.4.0/gems/pluggaloid-1.1.1/lib/pluggaloid/event.rb:97:in `block (2 levels) in call_all_listeners'
from /home/toshi/Projects/mikutter/vendor/bundle/ruby/2.4.0/gems/pluggaloid-1.1.1/lib/pluggaloid/event.rb:96:in `each'
from /home/toshi/Projects/mikutter/vendor/bundle/ruby/2.4.0/gems/pluggaloid-1.1.1/lib/pluggaloid/event.rb:96:in `block in call_all_listeners'
from /home/toshi/Projects/mikutter/vendor/bundle/ruby/2.4.0/gems/pluggaloid-1.1.1/lib/pluggaloid/event.rb:95:in `catch'
from /home/toshi/Projects/mikutter/vendor/bundle/ruby/2.4.0/gems/pluggaloid-1.1.1/lib/pluggaloid/event.rb:95:in `call_all_listeners'
from /home/toshi/Projects/mikutter/vendor/bundle/ruby/2.4.0/gems/pluggaloid-1.1.1/lib/pluggaloid/event.rb:39:in `block in call'
from /home/toshi/Projects/mikutter/vendor/bundle/ruby/2.4.0/gems/delayer-0.0.2/lib/delayer/procedure.rb:24:in `run'
from /home/toshi/Projects/mikutter/vendor/bundle/ruby/2.4.0/gems/delayer-0.0.2/lib/delayer/extend.rb:58:in `run_once'
from /home/toshi/Projects/mikutter/vendor/bundle/ruby/2.4.0/gems/delayer-0.0.2/lib/delayer/extend.rb:30:in `run'
from /home/toshi/Projects/mikutter/vendor/bundle/ruby/2.4.0/gems/delayer-0.0.2/lib/delayer.rb:43:in `method_missing'
from /home/toshi/Projects/mikutter/core/plugin/gtk/delayer.rb:10:in `block in boot'
from /home/toshi/Projects/mikutter/core/plugin/gtk/mainloop.rb:10:in `main'
from /home/toshi/Projects/mikutter/core/plugin/gtk/mainloop.rb:10:in `mainloop'
from /home/toshi/Projects/mikutter/mikutter.rb:67:in `boot!'
from /home/toshi/Projects/mikutter/mikutter.rb:96:in `<main>'
toshi_a 初音 さんがほぼ7年前に更新
クラッシュしている理由¶
IdentityExtend#.findbyid は、 USE_LOCAL_ONLY モードと USE_ALL モードの２種類があり、後者の場合はカレントスレッドでTwitter APIにアクセスするので、メインスレッドから呼び出そうとするとクラッシュします。MikuTwitterがメインスレッドで通信を要求されたらクラッシュするのは仕様なので、そもそもここで Message.findbyid を呼び出すべきではありません。
source:core/lib/mikutwitter/api_shortcuts.rb@fb3fbc235992c965e63217301707547fcdbc1129#L125 で Message.generate を呼んでいて、それが
source:core/lib/retriever/model/identity.rb@fb3fbc235992c965e63217301707547fcdbc1129#L26 にあるとおり、 USE_ALL モードでfindbyidを呼び出しているのですが、本来Twitter APIには返信先のツイートIDを渡せばいいだけなので、 Message.findbyid を呼ぶのはバグです。
今まで動いていた理由¶
Message.findbyid は、最初にMemoryというオンメモリのDataSourceを利用します。これが今回のクラッシュの引き金となる rev:d4a3463d で変更された箇所で、Modelに対してto_iが呼ばれている場所です。
リプライ先のMessageは絶対にメモリ上にあるので、ここでMessageインスタンスが見つかり、APIリクエストは実行されませんでした。
しかし source:core/lib/mikutwitter/api_shortcuts.rb@fb3fbc235992c965e63217301707547fcdbc1129#L125 では Message.generate に Message Modelを渡しており、結果findbyidの対象となるIDがMessage自身になっていました。今回の変更ではメモリ状を探索するキーがIDからMessage Modelになったためキャッシュヒットせず、APIアクセスにフォールバックされてクラッシュしています。
修正案¶
findbyidの引数はIDっぽいオブジェクトを渡すべきで、結果として期待しているModelを渡しているのは完全に無駄なので、やめるべきです。
しかしサードパーティプラグインがこの挙動を使っている可能性も否定できないので、 rev:d4a3463d はrevertして、findbyidの挙動を元に戻すべきだと思います。
そのうえで、今回のようなfindbyidの使い方をしている Message.generate については、引数がMessage Modelなら引数をそのまま返すような修正をすれば、リプライ時にMessage#to_iが呼ばれることはなく、findbyidにMessage Modelのインスタンスを渡しても、警告が表示された上で従来通りに動作します。
クラッシュしている状態ではリリースできないので、俺がやっておきます。
toshi_a 初音 さんがほぼ7年前に更新
そもそもMessage.generateがこの一箇所しか呼ばれておらず、他のプラグインから呼ばれることも想定していないので、Message.generate自体がdeprecatedにするほうが良さそう
toshi_a 初音 さんがほぼ7年前に更新
- ステータス を 実装待ち から パッチ適用待ち に変更
- 担当者 を toshi_a 初音 から あひる 家鴨 に変更
topic/1047-crash-in-sending-reply にpushしました。良さそうだったらhotfix/3.5にmergeしてください。