この記事は、Ruby Advent Calendar 2020 の 8 日目の記事です。
昨日は @udzura さんの mruby 本を支えた Continuous Delivery でした。
概要
Thread#raise を使う場合に ensure が実行されないケースがあります。
代表例としては Timeout.timeout があります。
Rails をはじめとするライブラリはクリーンアップ処理を ensure に頼っているケースが多いので、リクエスト中に Timeout.timeout した場合、そのプロセスは後続リクエストを正常に処理できない可能性があります。
次のリクエストを処理する前に終了させたほうがよいです。
rack middleware やライブラリ内でこれらを使用していないかには注意したほうがよいでしょう。
例えば、rack-timeout を使ってデフォルト設定のままの場合には、必ず term_on_timeout: true にしましょう。
詳細
次のコードを実行してみてください。
ensure できなかった場合にのみ無限ループから抜けます。
require 'timeout'
class Foo
def stack
@stack ||= []
end
def foo
stack.push('hi')
ensure
stack.pop
end
end
foo = nil
loop do
foo = Foo.new
begin
Timeout.timeout(Float::EPSILON) do
loop do
foo.foo
end
end
rescue Timeout::Error
break unless foo.stack.empty?
end
end
p foo.stack
# => ["hi"]
まさにこのような場合でも確実に ensure するために Thread.handle_interrupt があります。
require 'timeout'
class Foo
def stack
@stack ||= []
end
def foo
Thread.handle_interrupt(Timeout::Error => :never) do
begin
stack.push('hi')
ensure
stack.pop
end
end
end
end
foo = nil
loop do
foo = Foo.new
begin
Timeout.timeout(Float::EPSILON) do
loop do
foo.foo
end
end
rescue Timeout::Error
break unless foo.stack.empty?
end
end
p foo.stack
# ここまでこない
つまり、ensure の位置でこの Thread.handle_interrupt をアプリケーションコードとライブラリのコードすべてに書いて回る必要があります。もっと言うと、すべての行でいつでも例外が上がってくるかもしれないということなので、そのようなことが起きても内部状態が壊れないよう考慮したコーディングが必要になります。
これについて、Rails ではきりがないため考慮しないこととしています。
ところで、ActiveRecord は scope 周りで ensure を利用している部分があります。
これを利用して、Timeout 前のリクエストで投げようとしていたクエリの条件を Timeout 後のスコープに入れ込むことが可能になります。
つまり、
User.count
というコードを、
User.where(name: "foo").count
に化けさせることができます。
以下が再現させるコードの例です。
保存して $ ruby foo.rb のようにすればそのまま実行できます。
require "bundler/inline"
gemfile do
source "https://rubygems.org"
gem "rails", "6.0.3"
gem "sqlite3"
gem "rack-timeout", require: "rack/timeout/base"
end
require "rack/test"
require "active_record/railtie"
require "action_controller/railtie"
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(File::NULL)
ActiveRecord::Base.logger.level = Logger::ERROR
ActiveRecord::Schema.define do
create_table :users, force: true do |t|
t.string :name
end
end
class User < ActiveRecord::Base
scope :sleep_a_while, -> { all.tap { sleep(rand / 1000000.0) } }
end
User.create!(name: "foo")
User.create!(name: "bar")
class TestApp < Rails::Application
config.root = __dir__
config.hosts << "example.org"
config.session_store :cookie_store, key: "cookie_store_key"
secrets.secret_key_base = "secret_key_base"
config.logger = ActiveRecord::Base.logger
Rails.logger = config.logger
config.middleware.insert_before Rack::Runtime, Rack::Timeout, service_timeout: 0.1
config.exceptions_app = ->(*) { [500, {}, ['']] }
routes.draw do
get "/" => "test#index"
get "/timeout" => "test#timeout"
end
end
class TestController < ActionController::Base
include Rails.application.routes.url_helpers
def index
render plain: User.count
end
def timeout
loop do
User.where(name: "foo").sleep_a_while
end
end
end
require "minitest/autorun"
class BugTest < Minitest::Test
include Rack::Test::Methods
def test_index
get "/"
# User.count が 2 であることを確認
assert_equal "2", last_response.body
until last_response.body == "1"
get "/timeout"
get "/"
end
get "/"
# User.count の結果は常に 2 でないといけないが、1 になっているためテストは失敗する
assert_equal "2", last_response.body
end
private
def app
Rails.application
end
end
このように、ensure に頼って書かれているコード全般が Timeout.timeout と組み合わせることで壊れる可能性があります。
その他
8年前に ko1 さんが ensure と Timeout.timeout の組み合わせの危険性についてアドベントカレンダーで書かれていました。
http://atdot.net/~ko1/diary/edit_comment.cgi?mode=long&year=2012&month=12&day=6
また、rack-timeout ではこういった timeout のリスクについて記しています。
https://github.com/sharpstone/rack-timeout/blob/f4b14a534b37d425ec4dba10d9cacf29ba012f9f/doc/risks.md
とはいえ、rack-timeout を使うにあたってはデフォルトで term_on_timeout: true のほうが望ましいと思われるので issue を立てています。
https://github.com/sharpstone/rack-timeout/issues/169