ensure は実行されるとは限らない

この記事は、[Ruby Advent Calendar 2020](https://qiita.com/advent-calendar/2020/ruby) の 8 日目の記事です。
昨日は @udzura さんの [mruby 本を支えた Continuous Delivery](https://udzura.hatenablog.jp/entry/2020/12/07/085933) でした。

## 概要

`Thread#raise` を使う場合に `ensure` が実行されないケースがあります。
代表例としては `Timeout.timeout` があります。

Rails をはじめとするライブラリはクリーンアップ処理を `ensure` に頼っているケースが多いので、リクエスト中に `Timeout.timeout` した場合、そのプロセスは後続リクエストを正常に処理できない可能性があります。
次のリクエストを処理する前に終了させたほうがよいです。

rack middleware やライブラリ内でこれらを使用していないかには注意したほうがよいでしょう。
例えば、rack-timeout を使ってデフォルト設定のままの場合には、必ず `term_on_timeout: true` にしましょう。

## 詳細

次のコードを実行してみてください。
`ensure` できなかった場合にのみ無限ループから抜けます。

```ruby
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` があります。

https://docs.ruby-lang.org/ja/master/method/Thread/s/handle_interrupt.html

```ruby
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 ではきりがないため考慮しないこととしています。

https://github.com/rails/rails/pull/17607#issuecomment-70538060

ところで、ActiveRecord は scope 周りで `ensure` を利用している部分があります。 

https://github.com/rails/rails/blob/9b6008924d527b61b11677a78542a7b0fd4d80bf/activerecord/lib/active_record/relation.rb#L812-L813

これを利用して、Timeout 前のリクエストで投げようとしていたクエリの条件を Timeout 後のスコープに入れ込むことが可能になります。

つまり、

```ruby
User.count
```

というコードを、

```ruby
User.where(name: "foo").count
```

に化けさせることができます。

以下が再現させるコードの例です。
保存して `$ ruby foo.rb` のようにすればそのまま実行できます。

```ruby
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