Remove use of json gem (#2479)

* Add basic JSON serializer

For now, it only handles Arrays of Integers, but we'll extend it to
support all of the common types

* Serialize Strings

* Escape quotes in Strings

* Escape backslashes in Strings

* Serialize Hashes with String keys

* Extract method for serializing Strings

* Add test coverage for non-Hash non-Array JSON serialization

* Add test for unexpected key types

* Serialize Hashes with Symbol keys

* Raise on unexpected value types

* Serialize boolean values

* Serialize Floats

* Add module comment to Puma::JSON

* Update integration test to use fully-qualfied JSON module reference

* Remove json gem dependency from /stats status server response

Fixes a bug where requesting `/stats` from the status server would cause
subsequent phased restarts to fail when upgrading/downgrading the json
gem.

* Run gc_stats tests on JRuby

These were disabled at some point on JRuby, but they seem to run fine.
Importantly, this test ensures that a call to `/gc-stats` returns
well-formed JSON on JRuby, where the value of `GC.stat` contains nested
structures.

* Remove json gem dependency from /gc-stats status server response

Fixes a bug where requesting `/gc-stats` from the status server would cause
subsequent phased restarts to fail when upgrading/downgrading the json
gem.

* Remove json gem from /thread-backtraces status server response

Fixes a bug where requesting `/thread-backtraces` from the status server
would cause subsequent phased restarts to fail when
upgrading/downgrading the json gem.

* Remove json gem dependency from Puma.stats

Fixes a bug where accessing `Puma.stats` would cause subsequent phased
restarts to fail when upgrading/downgrading the json gem.

* Fix test name in json test

Co-authored-by: rmacklin <1863540+rmacklin@users.noreply.github.com>

* Add History entry

* Add test for exceptions on values of unexpected types

* Update test name for additional clarity

* Reorder cases to match order in ECMA-404

* Allow all serializable inputs in Puma::JSON::serialize

The pervious implementation was based on and older JSON standard which
defined JSON texts to be either objects or arrays. Modern JSON standands
allow all JSON values to be valid JSON texts.

* Update JSON tests to test value types directly

* Reorder tests to roughly match source order

* Add test for serializing integers as JSON

* Serialize nil as null

* Use block form of gsub instead of hash form

* Escape control characters as required by ECMA-404

* Collapse handling of Symbol and String into one case

* Extract constants used in string serialization

* Remove superflous else case

* Use stringio for incremental JSON construction

* Extract test helper for testing JSON serialization

* Assert that strings generated by Puma::JSON roundtrip when using ::JSON

* Use a recent version of the json gem in tests

`::JSON.parse` doesn't handle JSON texts other than objects and arrays
in old versions

* Handle default expected_roundtrip more explicitly for clarity

Co-authored-by: rmacklin <1863540+rmacklin@users.noreply.github.com>

Gemfile

@@ -5,6 +5,7 @@ gemspec
 gem "rdoc"
 gem "rake-compiler", "~> 1.1.1"
 
+gem "json", "~> 2.3"
 gem "nio4r", "~> 2.0"
 gem "rack", "~> 1.6"
 gem "minitest", "~> 5.11"

History.md

@@ -13,6 +13,7 @@
   * Ignore illegal (by Rack spec) response header ([#2439])
   * Close idle connections immediately on shutdown ([#2460])
   * Fix some instances of phased restart errors related to the `json` gem (#2473)
+  * Remove use of `json` gem to fix phased restart errors (#2479)
 
 ## 5.0.4 / 2020-10-27
 

lib/puma.rb

@@ -12,6 +12,7 @@ require 'thread'
 
 require 'puma/puma_http11'
 require 'puma/detect'
+require 'puma/json'
 
 module Puma
   autoload :Const, 'puma/const'
@@ -25,8 +26,7 @@ module Puma
 
   # @!attribute [rw] stats_object
   def self.stats
-    require 'json'
-    @get_stats.stats.to_json
+    Puma::JSON.generate @get_stats.stats
   end
 
   # @!attribute [r] stats_hash

lib/puma/app/status.rb

@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+require 'puma/json'
 
 module Puma
   module App
@@ -22,10 +23,6 @@ module Puma
           return rack_response(403, 'Invalid auth token', 'text/plain')
         end
 
-        if env['PATH_INFO'] =~ /\/(gc-stats|stats|thread-backtraces)$/
-          require 'json'
-        end
-
         # resp_type is processed by following case statement, return
         # is a number (status) or a string used as the body of a 200 response
         resp_type =
@@ -49,17 +46,17 @@ module Puma
             GC.start ; 200
 
           when 'gc-stats'
-            GC.stat.to_json
+            Puma::JSON.generate GC.stat
 
           when 'stats'
-            @launcher.stats.to_json
+            Puma::JSON.generate @launcher.stats
 
           when 'thread-backtraces'
             backtraces = []
             @launcher.thread_status do |name, backtrace|
               backtraces << { name: name, backtrace: backtrace }
             end
-            backtraces.to_json
+            Puma::JSON.generate backtraces
 
           else
             return rack_response(404, "Unsupported action", 'text/plain')

lib/puma/json.rb

@@ -0,0 +1,96 @@
+# frozen_string_literal: true
+require 'stringio'
+
+module Puma
+
+  # Puma deliberately avoids the use of the json gem and instead performs JSON
+  # serialization without any external dependencies. In a puma cluster, loading
+  # any gem into the puma master process means that operators cannot use a
+  # phased restart to upgrade their application if the new version of that
+  # application uses a different version of that gem. The json gem in
+  # particular is additionally problematic because it leverages native
+  # extensions. If the puma master process relies on a gem with native
+  # extensions and operators remove gems from disk related to old releases,
+  # subsequent phased restarts can fail.
+  #
+  # The implementation of JSON serialization in this module is not designed to
+  # be particularly full-featured or fast. It just has to handle the few places
+  # where Puma relies on JSON serialization internally.
+
+  module JSON
+    QUOTE = /"/
+    BACKSLASH = /\\/
+    CONTROL_CHAR_TO_ESCAPE = /[\x00-\x1F]/ # As required by ECMA-404
+    CHAR_TO_ESCAPE = Regexp.union QUOTE, BACKSLASH, CONTROL_CHAR_TO_ESCAPE
+
+    class SerializationError < StandardError; end
+
+    class << self
+      def generate(value)
+        StringIO.open do |io|
+          serialize_value io, value
+          io.string
+        end
+      end
+
+      private
+
+      def serialize_value(output, value)
+        case value
+        when Hash
+          output << '{'
+          value.each_with_index do |(k, v), index|
+            output << ',' if index != 0
+            serialize_object_key output, k
+            output << ':'
+            serialize_value output, v
+          end
+          output << '}'
+        when Array
+          output << '['
+          value.each_with_index do |member, index|
+            output << ',' if index != 0
+            serialize_value output, member
+          end
+          output << ']'
+        when Integer, Float
+          output << value.to_s
+        when String
+          serialize_string output, value
+        when true
+          output << 'true'
+        when false
+          output << 'false'
+        when nil
+          output << 'null'
+        else
+          raise SerializationError, "Unexpected value of type #{value.class}"
+        end
+      end
+
+      def serialize_string(output, value)
+        output << '"'
+        output << value.gsub(CHAR_TO_ESCAPE) do |character|
+          case character
+          when BACKSLASH
+            '\\\\'
+          when QUOTE
+            '\\"'
+          when CONTROL_CHAR_TO_ESCAPE
+            '\u%.4X' % character.ord
+          end
+        end
+        output << '"'
+      end
+
+      def serialize_object_key(output, value)
+        case value
+        when Symbol, String
+          serialize_string output, value.to_s
+        else
+          raise SerializationError, "Could not serialize object of type #{value.class} as object key"
+        end
+      end
+    end
+  end
+end

test/config/prune_bundler_print_json_defined.rb

@@ -1,4 +1,4 @@
 prune_bundler true
 before_fork do
-  puts "defined?(JSON): #{defined?(JSON).inspect}"
+  puts "defined?(::JSON): #{defined?(::JSON).inspect}"
 end

test/test_cli.rb

@@ -322,7 +322,6 @@ class TestCLI < Minitest::Test
   end
 
   def test_control_gc_stats_tcp
-    skip_on :jruby, suffix: " - Hitting /gc route does not increment count"
     uri  = "tcp://127.0.0.1:#{UniquePort.call}/"
     cntl_port = UniquePort.call
     cntl = "tcp://127.0.0.1:#{cntl_port}/"
@@ -331,7 +330,6 @@ class TestCLI < Minitest::Test
   end
 
   def test_control_gc_stats_unix
-    skip_on :jruby, suffix: " - Hitting /gc route does not increment count"
     skip UNIX_SKT_MSG unless UNIX_SKT_EXIST
 
     uri  = "unix://#{@tmp_path2}"

test/test_integration_cluster.rb

@@ -280,7 +280,7 @@ RUBY
     cli_server "-w #{workers} -C test/config/prune_bundler_print_json_defined.rb test/rackup/hello.ru"
 
     line = @server.gets
-    assert_match(/defined\?\(JSON\): nil/, line)
+    assert_match(/defined\?\(::JSON\): nil/, line)
   end
 
   def test_application_is_loaded_exactly_once_if_using_preload_app

test/test_json.rb

@@ -0,0 +1,107 @@
+require_relative "helper"
+require "json"
+require "puma/json"
+
+class TestJSON < Minitest::Test
+  parallelize_me! unless JRUBY_HEAD
+
+  def test_json_generates_string_for_hash_with_string_keys
+    value = { "key" => "value" }
+    assert_puma_json_generates_string '{"key":"value"}', value
+  end
+
+  def test_json_generates_string_for_hash_with_symbol_keys
+    value = { key: 'value' }
+    assert_puma_json_generates_string '{"key":"value"}', value, expected_roundtrip: { "key" => "value" }
+  end
+
+  def test_generate_raises_error_for_unexpected_key_type
+    value = { [1] => 'b' }
+    ex = assert_raises Puma::JSON::SerializationError do
+      Puma::JSON.generate value
+    end
+    assert_equal 'Could not serialize object of type Array as object key', ex.message
+  end
+
+  def test_json_generates_string_for_array_of_integers
+    value = [1, 2, 3]
+    assert_puma_json_generates_string '[1,2,3]', value
+  end
+
+  def test_json_generates_string_for_array_of_strings
+    value = ["a", "b", "c"]
+    assert_puma_json_generates_string '["a","b","c"]', value
+  end
+
+  def test_json_generates_string_for_nested_arrays
+    value = [1, [2, [3]]]
+    assert_puma_json_generates_string '[1,[2,[3]]]', value
+  end
+
+  def test_json_generates_string_for_integer
+    value = 42
+    assert_puma_json_generates_string '42', value
+  end
+
+  def test_json_generates_string_for_float
+    value = 1.23
+    assert_puma_json_generates_string '1.23', value
+  end
+
+  def test_json_escapes_strings_with_quotes
+    value = 'a"'
+    assert_puma_json_generates_string '"a\""', value
+  end
+
+  def test_json_escapes_strings_with_backslashes
+    value = 'a\\'
+    assert_puma_json_generates_string '"a\\\\"', value
+  end
+
+  def test_json_escapes_strings_with_null_byte
+    value = "\x00"
+    assert_puma_json_generates_string '"\u0000"', value
+  end
+
+  def test_json_escapes_strings_with_unicode_information_separator_one
+    value = "\x1f"
+    assert_puma_json_generates_string '"\u001F"', value
+  end
+
+  def test_json_generates_string_for_true
+    value = true
+    assert_puma_json_generates_string 'true', value
+  end
+
+  def test_json_generates_string_for_false
+    value = false
+    assert_puma_json_generates_string 'false', value
+  end
+
+  def test_json_generates_string_for_nil
+    value = nil
+    assert_puma_json_generates_string 'null', value
+  end
+
+  def test_generate_raises_error_for_unexpected_value_type
+    value = /abc/
+    ex = assert_raises Puma::JSON::SerializationError do
+      Puma::JSON.generate value
+    end
+    assert_equal 'Unexpected value of type Regexp', ex.message
+  end
+
+  private
+
+  def assert_puma_json_generates_string(expected_output, value_to_serialize, expected_roundtrip: nil)
+    actual_output = Puma::JSON.generate(value_to_serialize)
+    assert_equal expected_output, actual_output
+
+    if value_to_serialize.nil?
+      assert_nil ::JSON.parse(actual_output)
+    else
+      expected_roundtrip ||= value_to_serialize
+      assert_equal expected_roundtrip, ::JSON.parse(actual_output)
+    end
+  end
+end

test/test_worker_gem_independence.rb

@@ -27,9 +27,66 @@ class TestWorkerGemIndependence < TestIntegration
                                              new_version: '2.3.0'
   end
 
+  def test_changing_json_version_during_phased_restart_after_querying_stats_from_status_server
+    @control_tcp_port = UniquePort.call
+    server_opts = "--control-url tcp://#{HOST}:#{@control_tcp_port} --control-token #{TOKEN}"
+    before_restart = ->() do
+      cli_pumactl "stats"
+    end
+
+    change_gem_version_during_phased_restart server_opts: server_opts,
+                                             before_restart: before_restart,
+                                             old_app_dir: 'worker_gem_independence_test/old_json',
+                                             old_version: '2.3.1',
+                                             new_app_dir: 'worker_gem_independence_test/new_json',
+                                             new_version: '2.3.0'
+  end
+
+  def test_changing_json_version_during_phased_restart_after_querying_gc_stats_from_status_server
+    @control_tcp_port = UniquePort.call
+    server_opts = "--control-url tcp://#{HOST}:#{@control_tcp_port} --control-token #{TOKEN}"
+    before_restart = ->() do
+      cli_pumactl "gc-stats"
+    end
+
+    change_gem_version_during_phased_restart server_opts: server_opts,
+                                             before_restart: before_restart,
+                                             old_app_dir: 'worker_gem_independence_test/old_json',
+                                             old_version: '2.3.1',
+                                             new_app_dir: 'worker_gem_independence_test/new_json',
+                                             new_version: '2.3.0'
+  end
+
+  def test_changing_json_version_during_phased_restart_after_querying_thread_backtraces_from_status_server
+    @control_tcp_port = UniquePort.call
+    server_opts = "--control-url tcp://#{HOST}:#{@control_tcp_port} --control-token #{TOKEN}"
+    before_restart = ->() do
+      cli_pumactl "thread-backtraces"
+    end
+
+    change_gem_version_during_phased_restart server_opts: server_opts,
+                                             before_restart: before_restart,
+                                             old_app_dir: 'worker_gem_independence_test/old_json',
+                                             old_version: '2.3.1',
+                                             new_app_dir: 'worker_gem_independence_test/new_json',
+                                             new_version: '2.3.0'
+  end
+
+  def test_changing_json_version_during_phased_restart_after_accessing_puma_stats_directly
+    change_gem_version_during_phased_restart old_app_dir: 'worker_gem_independence_test/old_json_with_puma_stats_after_fork',
+                                             old_version: '2.3.1',
+                                             new_app_dir: 'worker_gem_independence_test/new_json_with_puma_stats_after_fork',
+                                             new_version: '2.3.0'
+  end
+
   private
 
-  def change_gem_version_during_phased_restart(old_app_dir:, new_app_dir:, old_version:, new_version:)
+  def change_gem_version_during_phased_restart(old_app_dir:,
+                                               new_app_dir:,
+                                               old_version:,
+                                               new_version:,
+                                               server_opts: '',
+                                               before_restart: nil)
     skip_unless_signal_exist? :USR1
 
     set_release_symlink File.expand_path(old_app_dir, __dir__)
@@ -38,7 +95,7 @@ class TestWorkerGemIndependence < TestIntegration
       with_unbundled_env do
         system("bundle config --local path vendor/bundle", out: File::NULL)
         system("bundle install", out: File::NULL)
-        cli_server '--prune-bundler -w 1'
+        cli_server "--prune-bundler -w 1 #{server_opts}"
       end
     end
 
@@ -46,6 +103,8 @@ class TestWorkerGemIndependence < TestIntegration
     initial_reply = read_body(connection)
     assert_equal old_version, initial_reply
 
+    before_restart.call if before_restart
+
     set_release_symlink File.expand_path(new_app_dir, __dir__)
     Dir.chdir(current_release_symlink) do
       with_unbundled_env do

test/worker_gem_independence_test/new_json_with_puma_stats_after_fork/Gemfile

@@ -0,0 +1,4 @@
+source "https://rubygems.org"
+
+gem 'puma', path: '../../..'
+gem 'json', '= 2.3.0'

test/worker_gem_independence_test/new_json_with_puma_stats_after_fork/config.ru

@@ -0,0 +1,2 @@
+require 'json'
+run lambda { |env| [200, {'Content-Type'=>'text/plain'}, [JSON::VERSION]] }

test/worker_gem_independence_test/new_json_with_puma_stats_after_fork/config/puma.rb

@@ -0,0 +1,2 @@
+directory File.expand_path("../../current", __dir__)
+after_worker_fork { Puma.stats }

test/worker_gem_independence_test/old_json_with_puma_stats_after_fork/Gemfile

@@ -0,0 +1,4 @@
+source "https://rubygems.org"
+
+gem 'puma', path: '../../..'
+gem 'json', '= 2.3.1'

test/worker_gem_independence_test/old_json_with_puma_stats_after_fork/config.ru

@@ -0,0 +1,2 @@
+require 'json'
+run lambda { |env| [200, {'Content-Type'=>'text/plain'}, [JSON::VERSION]] }

test/worker_gem_independence_test/old_json_with_puma_stats_after_fork/config/puma.rb

@@ -0,0 +1,2 @@
+directory File.expand_path("../../current", __dir__)
+after_worker_fork { Puma.stats }