最適化 #1047
closed
deprecated な User#to_i が使われている
Added by あひる 家鴨 over 8 years ago. Updated over 8 years ago.
Description
Retriever::Model::Memory もしくは Diva::Model::Memory で Retriever::Model::User の to_i メソッドが使われている
Updated by あひる 家鴨 over 8 years ago
- Status changed from 新規 to パッチ適用待ち
- Assignee changed from あひる 家鴨 to toshi_a 初音
- プラグイン名 changed from core/ lib/diva_hacks/model/memory.rb to 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 なだけではありますが）
この場合、どちらのブランチに向けてコミットすればよろしいでしょうか。
Updated by toshi_a 初音 over 8 years ago
- Status changed from パッチ適用待ち to レビュー待ち
- Assignee changed from toshi_a 初音 to あひる 家鴨
hotfix/3.5 にmergeしました。一応確認お願いします。
Updated by toshi_a 初音 over 8 years ago
- Status changed from 終了 to 実装待ち
- Assignee changed from あひる 家鴨 to 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>'
Updated by toshi_a 初音 over 8 years ago
クラッシュしている理由¶
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のインスタンスを渡しても、警告が表示された上で従来通りに動作します。
クラッシュしている状態ではリリースできないので、俺がやっておきます。
Updated by toshi_a 初音 over 8 years ago
そもそもMessage.generateがこの一箇所しか呼ばれておらず、他のプラグインから呼ばれることも想定していないので、Message.generate自体がdeprecatedにするほうが良さそう
Updated by toshi_a 初音 over 8 years ago
- Status changed from 実装待ち to パッチ適用待ち
- Assignee changed from toshi_a 初音 to あひる 家鴨
topic/1047-crash-in-sending-reply にpushしました。良さそうだったらhotfix/3.5にmergeしてください。
Updated by あひる 家鴨 over 8 years ago
- Status changed from パッチ適用待ち to レビュー待ち
問題なさそうだったので hotfix/3.5 にマージしました。
一応確認をお願いします。