Remove use of json gem #2479

cjlarose · 2020-11-03T07:08:52Z

Description

Fixes #2471. The changes proposed in this PR in particular are described in a comment on the same issue. This PR introduces a new Puma::JSON module, which is meant to handle the specific places where Puma needs to perform JSON serialization.

Your checklist for this pull request

I have reviewed the guidelines for contributing to this repository.
I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
I have added appropriate tests if this PR fixes a bug or adds a feature.
My pull request is 100 lines added/removed or less so that it can be easily reviewed.
If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
I have updated the documentation accordingly.
All new and existing tests passed, including Rubocop.

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

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

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.

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

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

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

nateberkopec · 2020-11-03T16:58:07Z

lib/puma/json.rb

@@ -0,0 +1,85 @@
+# frozen_string_literal: true
+
+module Puma


You really went above and beyond here. Better than what we had in 3.x by a mile!

nateberkopec · 2020-11-03T16:59:06Z

100% for the approach, will do a line by line review soon

rmacklin

This is great!

The integration tests you added should prevent the specific scenarios that currently make it impossible to upgrade/downgrade json during a phased restart. If I understand correctly, though, the test suite could still pass even if someone were to introduce a new usage of json in some other code path, right?

I'm not sure there's much we can do about that (unless we want to ban the usage of require 'json' in general...) but just wanted to confirm that we'd still want to be on the lookout for new usages of json that could cause problems with phased restarts.

rmacklin · 2020-11-03T14:50:15Z

lib/puma/json.rb

@@ -40,6 +40,8 @@ def serialize_value(output, value)
          serialize_string output, value
        when Integer
          output << value.to_s
+        else
+          raise SerializationError, "Unexpected value of type #{value.class}"


Should we add a test for this case? (We already have tests for the other two SerializationError cases.)

Nice catch! 806f5c4

test/test_json.rb

rmacklin · 2020-11-03T18:19:10Z

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 fixed phased restart errors (#2479)


Minor typo here:

Suggested change

* Remove use of `json` gem to fixed phased restart errors (#2479)

* Remove use of `json` gem to fix phased restart errors (#2479)

Fixed up in 0bd5531

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

ekohl · 2020-11-04T17:13:17Z

lib/puma/json.rb

+        case value
+        when Array, Hash


Why is this needed? serialize_value already handles all types or raises a SerializationError if needed.

I think my implementation was based on an understanding of JSON according to RFC 4627, which requires that a valid JSON text is either an object or any array, but it looks like that requirement was dropped in RFC 7158. I'll update the implementation since now there's no need to handle Hash and Array in any special way.

Fixed in 8d05a2f

ekohl · 2020-11-04T17:15:03Z

lib/puma/json.rb

+      def generate(value)
+        case value
+        when Array, Hash
+          parts = []


Rather than an array, should it use StringIO? Not sure if there is a (performance) benefit for one or the other.

I considered using stringio at first since it seems like a nice way to construct a string incrementally without an explosion in extra space in that same way that the Array#<< and Array#join strategy does. I went with just a normal Array because even stringio isn't in Ruby Core and, just like json, users can technically change the version of stringio they're using in a phased restart, which would cause workers to fail to boot. That said puma already uses stringio internally, so this is already a bug, so I could go either way on this.

I can't imagine the performance improvement would be too significant, especially since again, this module isn't really on a hot path.

The reason I'd switch to stringio is if it would improve readability/maintainability. I'll play with it and see what it looks like.

Switched to StringIO in f818d65.

I ended up doing this after all because I think using the Array approach I was doing before stood out as unusual after thinking about it. If someone was trying to build a String incrementally while keep allocations to a minimum, a reader of the code would expect the author to have used stringio. Deviating from that expectation would suggest that the distinction was significant, but in this case, it isn't.

ekohl · 2020-11-04T17:16:54Z

lib/puma/json.rb

+    class SerializationError < StandardError; end
+
+    class << self
+      def generate(value)


Does it make sense to attempt using the native JSON module for those who don't use phased restarts? It'd be faster.

I think the cost of doing that might not be worth the benefit. Use of JSON serialization in puma isn't on a "hot" path, and there would have to be additional complexity to pass around the information such that Puma::JSON (or its callers) knew when it was appropriate to use the built-in ::JSON or the new Puma::JSON. If we get it wrong somewhere, that's an opportunity to re-introduce a regression.

MSP-Greg · 2020-11-04T18:05:44Z

lib/puma/json.rb

+        when Symbol
+          serialize_string output, value.to_s
+        when String
+          serialize_string output, value


In the past I used inspect for strings and symbols. Maybe replace serialize_string with inspect:

when Symbol, String output << value.to_s.inspect

or

when Symbol output << value.to_s.inspect when String output << value.inspect

I think it does all the needed processing?

EDIT: Just noticed the call ro serialize_string in serialize_value, that needs to be replaced also. Just tried the test locally, and inspect seems fine.

I'll dig deeper into this. I realized I'm not handling all of the different escape sequences correctly. Newline characters, for example, are written literally instead of as \n.

I'll have to double check that everything that .inspect emits is valid JSON. If it is, I'll use it, otherwise, I'll handle the special characters in a different way.

Thanks. Your comment said

# 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.

I suspect you know better than any of us what Puma sends to JSON.generate (having just reviewed it), so I'll trust your judgement as to what edge cases it should handle.

EDIT: I just checked (not exhaustively) arrays composed of strings and/or numbers, and inspect is equivalent to JSON.generate with a space between members...

Added 98b3408 to handle control characters correctly and added 28e9d8f to simplify the handling of String and Symbol.

AFAICT, there's no guarantee that .inspect produces valid JSON (or will always produce valid JSON into the future), and adding support for control characters explicitly ended up not being so bad. I think the cost of adding the cases for the different characters is small, and it makes the implementation more closely match the language of the specification. I prefer that to having to worry about some edge case down the road where .inspect produces something that JSON parsers don't like.

nateberkopec · 2020-11-04T19:09:18Z

Thank you so much for the reviews @MSP-Greg @ekohl and @rmacklin 🙇

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.

ekohl · 2020-11-06T12:33:48Z

test/test_json.rb

+require_relative "helper"
+require "puma/json"
+
+class TestJSON < Minitest::Test


For all of these, does it make sense to assert JSON.parse(Puma::JSON.generate(value)) == value?

Ensuring that Puma::JSON.generate returns strings that can by parsed by ::JSON.parse seems useful, for sure.

Updated in b905c34

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

ekohl · 2020-11-06T16:28:54Z

test/test_json.rb

+
+  private
+
+  def assert_puma_json_generates_string(expected_output, value_to_serialize, expected_roundtrip: value_to_serialize)


I've never seen this before. Does value_to_serialize refer to the value that was passed in as the second argument?

It seems to work that way, but I couldn't find that behavior being explicitly supported in any official Ruby documentation, so I made it a little more clear in 0bb2df3

cjlarose · 2020-11-08T08:26:08Z

I think I've addressed all of the feedback so far. Lemme know if there's anything else we should do!

ekohl

It's a complex PR so I may have missed something, but overall I think it's a good approach.

cjlarose added 19 commits November 2, 2020 22:51

Add basic JSON serializer

a4cd9fd

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

Serialize Strings

b941156

Escape quotes in Strings

b636e7a

Escape backslashes in Strings

b9fcd52

Serialize Hashes with String keys

f310f1c

Extract method for serializing Strings

c6c4db3

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

93d25eb

Add test for unexpected key types

69597fd

Serialize Hashes with Symbol keys

258b5ba

Raise on unexpected value types

8543082

Serialize boolean values

7af8c01

Serialize Floats

a6b4833

Add module comment to Puma::JSON

c1114b3

Update integration test to use fully-qualfied JSON module reference

19e1584

Remove json gem dependency from /stats status server response

d765aaa

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

c094cf8

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

2bea320

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

dfedb0c

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

e87d6c1

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

cjlarose force-pushed the remove-use-of-json-gem branch from fb660cc to e6547fc Compare November 3, 2020 07:09

cjlarose mentioned this pull request Nov 3, 2020

phased-restart errors after upgrading from Puma 3.12.6 to 5.0.4 #2471

Closed

nateberkopec added bug waiting-for-review Waiting on review from anyone labels Nov 3, 2020

nateberkopec reviewed Nov 3, 2020

View reviewed changes

cjlarose force-pushed the remove-use-of-json-gem branch from e6547fc to 8bab708 Compare November 3, 2020 18:18

rmacklin reviewed Nov 3, 2020

View reviewed changes

cjlarose and others added 2 commits November 3, 2020 10:42

Fix test name in json test

2bef0c4

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

Add History entry

0bd5531

cjlarose force-pushed the remove-use-of-json-gem branch from bc37311 to 9317230 Compare November 3, 2020 18:46

cjlarose force-pushed the remove-use-of-json-gem branch from 9317230 to 37641b5 Compare November 3, 2020 18:58

Update test name for additional clarity

28915e7

cjlarose force-pushed the remove-use-of-json-gem branch from 37641b5 to 28915e7 Compare November 3, 2020 21:55

ekohl reviewed Nov 4, 2020

View reviewed changes

MSP-Greg reviewed Nov 4, 2020

View reviewed changes

cjlarose added 12 commits November 4, 2020 19:22

Reorder cases to match order in ECMA-404

c21c074

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

8d05a2f

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

8f4be6f

Reorder tests to roughly match source order

42e84ab

Add test for serializing integers as JSON

b69e898

Serialize nil as null

a805af5

Use block form of gsub instead of hash form

3238e94

Escape control characters as required by ECMA-404

98b3408

Collapse handling of Symbol and String into one case

28e9d8f

Extract constants used in string serialization

0fa80ef

Remove superflous else case

6e7f3c9

Use stringio for incremental JSON construction

f818d65

cjlarose force-pushed the remove-use-of-json-gem branch from 513aab1 to f818d65 Compare November 6, 2020 04:03

ekohl reviewed Nov 6, 2020

View reviewed changes

cjlarose added 3 commits November 6, 2020 07:19

Extract test helper for testing JSON serialization

be2ad5a

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

b905c34

Use a recent version of the json gem in tests

1015bad

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

ekohl reviewed Nov 6, 2020

View reviewed changes

Handle default expected_roundtrip more explicitly for clarity

0bb2df3

ekohl approved these changes Nov 9, 2020

View reviewed changes

nateberkopec merged commit 64c0153 into puma:master Nov 10, 2020

yob mentioned this pull request Jan 4, 2021

Consider requiring json at runtime yob/puma-plugin-statsd#23

Closed

walro mentioned this pull request Jun 8, 2021

Introduction of Puma::JSON in 5.1.0 is "breaking" in some cases #2639

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove use of json gem #2479

Remove use of json gem #2479

cjlarose commented Nov 3, 2020

nateberkopec Nov 3, 2020

nateberkopec commented Nov 3, 2020

rmacklin left a comment

rmacklin Nov 3, 2020

cjlarose Nov 3, 2020 •

edited

rmacklin Nov 3, 2020

cjlarose Nov 3, 2020

ekohl Nov 4, 2020

cjlarose Nov 4, 2020 •

edited

cjlarose Nov 6, 2020

ekohl Nov 4, 2020

cjlarose Nov 4, 2020 •

edited

cjlarose Nov 4, 2020

cjlarose Nov 6, 2020 •

edited

ekohl Nov 4, 2020

cjlarose Nov 4, 2020

MSP-Greg Nov 4, 2020 •

edited

cjlarose Nov 4, 2020 •

edited

MSP-Greg Nov 4, 2020 •

edited

cjlarose Nov 6, 2020

nateberkopec commented Nov 4, 2020

ekohl Nov 6, 2020

cjlarose Nov 6, 2020

ekohl Nov 6, 2020

cjlarose Nov 6, 2020

cjlarose commented Nov 8, 2020

ekohl left a comment

	* Remove use of `json` gem to fixed phased restart errors (#2479)
	* Remove use of `json` gem to fix phased restart errors (#2479)


		private

		def assert_puma_json_generates_string(expected_output, value_to_serialize, expected_roundtrip: value_to_serialize)

Remove use of json gem #2479

Remove use of json gem #2479

Conversation

cjlarose commented Nov 3, 2020

Description

Your checklist for this pull request

Choose a reason for hiding this comment

nateberkopec commented Nov 3, 2020

rmacklin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjlarose Nov 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjlarose Nov 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjlarose Nov 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjlarose Nov 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSP-Greg Nov 4, 2020 • edited

Choose a reason for hiding this comment

cjlarose Nov 4, 2020 • edited

Choose a reason for hiding this comment

MSP-Greg Nov 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateberkopec commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjlarose commented Nov 8, 2020

ekohl left a comment

Choose a reason for hiding this comment

cjlarose Nov 3, 2020 •

edited

cjlarose Nov 4, 2020 •

edited

cjlarose Nov 4, 2020 •

edited

cjlarose Nov 6, 2020 •

edited

MSP-Greg Nov 4, 2020 •

edited

cjlarose Nov 4, 2020 •

edited

MSP-Greg Nov 4, 2020 •

edited