プロジェクト

全般

プロフィール

最適化 #1047

deprecated な User#to_i が使われている

あひる 家鴨7ヶ月前に追加. 6ヶ月前に更新.

ステータス:
終了
優先度:
通常
対象バージョン:
開始日:
2017-05-28
期日:
進捗率:

0%

プラグイン名:
core/ lib/retriever/model/memory.rb

説明

Retriever::Model::Memory もしくは Diva::Model::Memory で Retriever::Model::User の to_i メソッドが使われている

関係しているリビジョン

リビジョン d4a3463d (差分)
あひる 家鴨7ヶ月前に追加

deprecated な User#to_i を削除 refs #1047

リビジョン fc93fb02 (差分)
toshi_a 初音6ヶ月前に追加

Revert "deprecated な User#to_i を削除 refs #1047"

This reverts commit d4a3463d239e3268317ac50e1f7e38824cfb496d.

リビジョン 6660c458 (差分)
toshi_a 初音6ヶ月前に追加

Message.generateにMessageを渡したら、そのMessageを返す refs #1047

履歴

#1 あひる 家鴨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 なだけではありますが)
この場合、どちらのブランチに向けてコミットすればよろしいでしょうか。

#2 toshi_a 初音6ヶ月前に更新

  • ステータスパッチ適用待 から レビュー待ち に変更
  • 担当者toshi_a 初音 から あひる 家鴨 に変更

hotfix/3.5 にmergeしました。一応確認お願いします。

#3 あひる 家鴨6ヶ月前に更新

  • ステータスレビュー待ち から 終了 に変更

確認しました。
ありがとうございます。

#4 toshi_a 初音6ヶ月前に更新

  • ステータス終了 から 進行中 に変更
  • 担当者あひる 家鴨 から 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>'

#5 toshi_a 初音6ヶ月前に更新

クラッシュしている理由

IdentityExtend#.findbyid は、 USE_LOCAL_ONLY モードと USE_ALL モードの2種類があり、後者の場合はカレントスレッドでTwitter APIにアクセスするので、メインスレッドから呼び出そうとするとクラッシュします。MikuTwitterがメインスレッドで通信を要求されたらクラッシュするのは仕様なので、そもそもここで Message.findbyid を呼び出すべきではありません。

source:core/lib/mikutwitter/api_shortcuts.rb@fb3fbc235992c965e63217301707547fcdbc1129#L125Message.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のインスタンスを渡しても、警告が表示された上で従来通りに動作します。

クラッシュしている状態ではリリースできないので、俺がやっておきます。

#6 toshi_a 初音6ヶ月前に更新

そもそもMessage.generateがこの一箇所しか呼ばれておらず、他のプラグインから呼ばれることも想定していないので、Message.generate自体がdeprecatedにするほうが良さそう

#7 toshi_a 初音6ヶ月前に更新

  • ステータス進行中 から パッチ適用待 に変更
  • 担当者toshi_a 初音 から あひる 家鴨 に変更

topic/1047-crash-in-sending-reply にpushしました。良さそうだったらhotfix/3.5にmergeしてください。

#8 あひる 家鴨6ヶ月前に更新

  • ステータスパッチ適用待 から レビュー待ち に変更

問題なさそうだったので hotfix/3.5 にマージしました。
一応確認をお願いします。

#9 toshi_a 初音6ヶ月前に更新

  • ステータスレビュー待ち から 終了 に変更

オッラーンです

他の形式にエクスポート: Atom PDF