サービスのバグ発見をきっかけに初めてOSSにコントリビュートした話

はじめまして。エンジニアの辻本です。Casaの代理店である不動産管理会社・仲介会社向けのWEBサービスの開発を担当しています。

普段、Ruby / Ruby on Railsを用いてサービスを開発しているのですが、少し前に、サービス内のかなり稀にしか起きないバグを発見し、そのバグ対応をきっかけに、初めてOSSにコードを書いてPRを出しました。無事マージされたので、その経緯を書いていきたいと思います。

github.com

※ドキュメントを和訳するPRは以前出したことがあり、厳密に言うと初めてのPRではないです。コードを書いてPRを出すのは今回が初めてなので、タイトルは少し盛っているのをご了承ください！

サービス内のバグを発見

バグの原因となっているRubyのコードは下記のようなものでした。

module Foo
  module_function

  def call(params)
    @params = params

    # do_something
  end
end

すぐ気づく人は気づくかもしれませんが、スレッドセーフじゃないですね。module_functionによって、メソッドがモジュール関数となりますが、モジュール関数はモジュールの特異メソッドであるため、Foo.call(params)のように呼び出すことができ、このとき、モジュールのコンテキストで状態をもってしまっています。マルチスレッドな場合、スレッド間でモジュールの状態は共有されるため、一方でインスタンス変数を書き換えるともう一方のスレッドにも影響を与えてしまうということになります。

開発しているサービスではpumaを利用していますが、pumaはマルチスレッドで動作するため、かなり稀に複数リクエストがモジュールを同じタイミングで呼び出してしまいバグが発生していました。

スレッドセーフじゃないコードを書いてしまったことによって意図しない挙動が起きる簡単な例を以下に示します。

module Foo
  module_function

  def call(name, number)
    @number = number
    puts "#{name}\t#{@number}"

    sleep 1
    @number += 1

    puts "#{name}\t#{@number}"
  end
end

t1 = Thread.start { Foo.call("Alice", 1) }
t2 = Thread.start { Foo.call("Bob", 10) }
t1.join
t2.join

Aliceのスレッドでは開始時に1、終了時に2と出力するのを意図していますが、Bobのスレッドがcallの1行目でインスタンス変数を上書きしてしまうため、Aliceのスレッドの終了時には11が出力されてしまっています。

$ ruby main.rb
Alice   1
Bob 10
Alice   11
Bob 12

スレッドセーフじゃないコードを自動で発見できないか？

今回のバグ調査では、目視で対象を洗い出して対応したのですが、こういうのってRuboCopとかで発見できないのかな？と思い調べてみました。すると、rubocop-thread_safetyという、いかにもそれなgemを見つけました。

スレッド間で状態を共有してしまう例として、Class instance variables (@name in class context or class methods) というような説明もあったので、インスタンス変数を不適切に扱っているところを検出してくれそうです。

試しにgemを入れてrubocopを実行してみました。

$ bundle exec rubocop main.rb
Inspecting 1 file
.

1 file inspected, no offenses detected

残念ながら今回のケースは検出できませんでした。クラスメソッド内でのインスタンス変数の利用は検出してくれるようですが、モジュール関数内での利用は検出してくれないようでした。

OSSにPRを出してみた 🎉

残念ながらrubocop-thread_safetyではモジュール関数内でのインスタンス変数を検出してくれないようでしたが、クラスメソッド内でのインスタンス変数の検出などで似たようなことをやっているはずだし、コードベースも小規模で把握しやすそうなので、自分で実装してPRを出してみようと思い立ちました。

RuboCopのCustom Copの書き方を調べながら、頑張って実装して出したPRがこちらです。

github.com

↓のように怒ってくれるようになります。

$ bundle exec rubocop main.rb
Inspecting 1 file
C

Offenses:

main.rb:5:5: C: ThreadSafety/InstanceVariableInClassMethod: Avoid instance variables in class methods.
    @number = number
    ^^^^^^^
main.rb:6:22: C: ThreadSafety/InstanceVariableInClassMethod: Avoid instance variables in class methods.
    puts "#{name}\t#{@number}"
                     ^^^^^^^
main.rb:9:5: C: ThreadSafety/InstanceVariableInClassMethod: Avoid instance variables in class methods.
    @number += 1
    ^^^^^^^
main.rb:11:22: C: ThreadSafety/InstanceVariableInClassMethod: Avoid instance variables in class methods.
    puts "#{name}\t#{@number}"
                     ^^^^^^^

1 file inspected, 4 offenses detected

実装としては、インスタンス変数を利用している箇所に対し、モジュール関数内で呼ばれているかどうかを AST をたどって判定しています。Custom Copを実装するための抽象化された仕組みをRuboCop側が用意してくれているのもあり、既存のコードを見ながら頑張って何とか実装することができました。どちらかというと、PRの説明を書くのに苦労した覚えがあります。

おわりに

今回出したPRはその後無事マージされました！

ただし、マージ後にまだ新しいバージョンがリリースされていないので、リポジトリとブランチを指定する方法で利用する必要があります。よければぜひ使ってみてください。

gem "rubocop-thread_safety", git: "https://github.com/rubocop/rubocop-thread_safety.git", branch: "master"

なお、PRをやりとりしていた時点ではサードパーティのgemだったのですが、その後、リポジトリがフォークされてRuboCop Headquartersに移管されており、公式のgemとなっているようです。rubocop-railsやrubocop-rspecと同じ立ち位置ですね。