hanami / router

Ruby/Rack HTTP router
http://hanamirb.org
MIT License
362 stars 92 forks source link

Ensure to not accidentally cache response headers for HTTP 404 and 405 #201

Closed jodosha closed 4 years ago

jodosha commented 4 years ago

Bug

We internally used constants to store serialized Rack responses for HTTP 404 and 405.

NOT_FOUND = [404, { "Content-Length" => "9" }, ["Not Found"]].freeze
NOT_ALLOWED = [405, { "Content-Length" => "11" }, ["Not Allowed"]].freeze

Problems

Problem 1: forgot to dup

In case of 404, the method was:

def not_found
  NOT_FOUND
end

It was returning the constant, without duping. When the Rack middleware were modifying the response headers the second element of the NOT_FOUND constant was modified.

The following 404 response were receiving the same headers of the first request. 😱 It was accidentally caching the values because Hashes in Ruby are passed by reference. So the constant was mutated 🙀

Problem 2: lack of deep freeze

To prevent the escaped (see Java Concurrency in Practice book) reference to be accidentally mutated we used to freeze the constant.

But because Object#freeze doesn't freeze inner objects, the headers hash of NOT_FOUND was still mutable.


Two stupid mistakes of mine 🤦

Fix

The fix is simple: don't use constants.

The implementation of these methods just return inline values, so they can be safely mutated.


Performance

I discovered that inline duped constants are 1.83x slower than inline values.

#!/usr/bin/env ruby
# frozen_string_literal: true

require "benchmark/ips"

EMPTY_RACK_ENV = {}.freeze

Benchmark.ips do |x|
  x.report("const") { env = EMPTY_RACK_ENV.dup; env["PATH_INFO"] = "/" }
  x.report("inline") { env = {}; env["PATH_INFO"] = "/" }
  x.compare!
end

__END__

Results:

Warming up --------------------------------------
               const   369.253k i/100ms
              inline   676.286k i/100ms
Calculating -------------------------------------
               const      3.672M (± 2.5%) i/s -     18.463M in   5.031615s
              inline      6.718M (± 7.6%) i/s -     33.814M in   5.068016s

Comparison:
              inline:  6718490.2 i/s
               const:  3671607.2 i/s - 1.83x  (± 0.00) slower

Ruby:

    ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin16]

Hardware:

    Hardware Overview:

      Model Name: MacBook Pro
      Model Identifier: MacBookPro12,1
      Processor Name: Intel Core i7
      Processor Speed: 3,1 GHz
      Number of Processors: 1
      Total Number of Cores: 2
      L2 Cache (per Core): 256 KB
      L3 Cache: 4 MB
      Memory: 16 GB
      Boot ROM Version: 186.0.0.0.0
      SMC Version (system): 2.28f7

Software:

    System Software Overview:

      System Version: macOS 10.12.6 (16G2136)
      Kernel Version: Darwin 16.7.0
      Time since boot: 4 days 50 minutes