Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support br encoding/decoding in response bank #63

Merged
merged 4 commits into from
Apr 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
PATH
remote: .
specs:
response_bank (1.2.0)
response_bank (1.3.0)
brotli
msgpack

GEM
Expand Down Expand Up @@ -73,7 +74,9 @@ GEM
minitest (>= 5.1)
tzinfo (~> 2.0)
ast (2.4.2)
brotli (0.4.0)
Copy link

@ianks ianks Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel great about adding this as a dependency...

  1. There are known segfaults in the issue tracker that have been left unaddressed for a year+
  2. After grepping, there does not seem to be any testing performed GC.stress = true to ensure GC safety

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any alternatives you'd recommend?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but those segfaults are just type checking on the parameters. Since we control this side, is there a risk? Or is your concern more broad stability?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At risk of sounding Rust-pilled, it's more of a general concern about C code that we consider safe to adopt widely in production. At a bare minimum, I think we should set some basic rules:

  1. Should be tested against latest stable Ruby (this gem only tests up to 3.0)
  2. Leverages ruby_memcheck to test for memory issues
  3. Runs test suite with GC.stress = true to smoke out GC related issues before they make it into production

FWIW, all of the above applies to all native extensions, both C and Rust.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the gem is small enough that we could easily fix it. What is more worrying to me is ownership. It's easy to produce the fixes, but will they be merged / published?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so the fix is here: miyucy/brotli#44. Let's see if the owners merge it. But either way, it's the same owner than the snappy gem we use extensively across shopify, so if they're AWOL, we're fucked either way.

For the GC.stress = true tests, honestly that gem defines a single type, and it has a single reference, and we don't even use that codepath, so I'm not too worried here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the fix was merged in less than 20 minutes, so there's that.

builder (3.2.4)
coderay (1.1.3)
concurrent-ruby (1.2.2)
crass (1.0.6)
date (3.3.3)
Expand Down Expand Up @@ -116,6 +119,9 @@ GEM
parallel (1.22.1)
parser (3.2.1.1)
ast (~> 2.4.1)
pry (0.14.2)
coderay (~> 1.1)
method_source (~> 1.0)
racc (1.6.2)
rack (2.2.6.4)
rack-test (2.1.0)
Expand Down Expand Up @@ -186,6 +192,7 @@ PLATFORMS
DEPENDENCIES
minitest (>= 5.18.0)
mocha
pry
rails (~> 7.0.4)
rake
response_bank!
Expand Down
42 changes: 33 additions & 9 deletions lib/response_bank.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
require 'response_bank/railtie' if defined?(Rails)
require 'response_bank/response_cache_handler'
require 'msgpack'
require 'brotli'

module ResponseBank
class << self
Expand All @@ -29,17 +30,26 @@ def read_from_backing_cache_store(_env, cache_key, backing_cache_store: cache_st
backing_cache_store.read(cache_key, raw: true)
drinkbeer marked this conversation as resolved.
Show resolved Hide resolved
end

def compress(content)
io = StringIO.new
gz = Zlib::GzipWriter.new(io)
gz.write(content)
io.string
ensure
gz.close
def compress(content, encoding = "gzip")
case encoding
when 'gzip'
Zlib.gzip(content, level: Zlib::BEST_COMPRESSION)
when 'br'
Brotli.deflate(content, mode: :text, quality: 7)
else
raise ArgumentError, "Unsupported encoding: #{encoding}"
end
end

def decompress(content)
Zlib::GzipReader.new(StringIO.new(content)).read
def decompress(content, encoding = "gzip")
case encoding
when 'gzip'
Zlib.gunzip(content)
when 'br'
Brotli.inflate(content)
else
raise ArgumentError, "Unsupported encoding: #{encoding}"
end
end

def cache_key_for(data)
Expand All @@ -53,6 +63,9 @@ def cache_key_for(data)

key = %{#{key}:#{hash_value_str(data[:version])}} if data[:version]

# add the encoding to only the cache key but don't expose this detail in the entity_tag
key = %{#{key}:#{hash_value_str(data[:encoding])}} if data[:encoding] && data[:encoding] != "gzip"

key
when Array
data.inspect
Expand All @@ -67,6 +80,17 @@ def cache_key_for(data)
end
end

def check_encoding(env, default_encoding = 'br')
colinbendell marked this conversation as resolved.
Show resolved Hide resolved
if env['HTTP_ACCEPT_ENCODING'].to_s.include?('br')
'br'
elsif env['HTTP_ACCEPT_ENCODING'].to_s.include?('gzip')
'gzip'
else
# No encoding requested from client, but we still need to cache the page in server cache
default_encoding
end
end

private

def hash_value_str(data)
Expand Down
22 changes: 15 additions & 7 deletions lib/response_bank/middleware.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def initialize(app)

def call(env)
env['cacheable.cache'] = false
gzip = env['gzip'] = env['HTTP_ACCEPT_ENCODING'].to_s.include?("gzip")
content_encoding = env['response_bank.server_cache_encoding'] = ResponseBank.check_encoding(env)

status, headers, body = @app.call(env)

Expand All @@ -35,11 +35,15 @@ def call(env)
body.each { |part| body_string << part }
end

body_gz = ResponseBank.compress(body_string)
body_compressed = nil
if body_string && body_string != ""
headers['Content-Encoding'] = content_encoding
body_compressed = ResponseBank.compress(body_string, content_encoding)
end

cached_headers = headers.slice(*CACHEABLE_HEADERS)
# Store result
cache_data = [status, cached_headers, body_gz, timestamp]
cache_data = [status, cached_headers, body_compressed, timestamp]

ResponseBank.write_to_cache(env['cacheable.key']) do
payload = MessagePack.dump(cache_data)
Expand All @@ -51,11 +55,15 @@ def call(env)
)
end

# since we had to generate the gz version above already we may
# since we had to generate the compressed version already we may
# as well serve it if the client wants it
if gzip
headers['Content-Encoding'] = "gzip"
body = [body_gz]
if body_compressed
if env['HTTP_ACCEPT_ENCODING'].to_s.include?(content_encoding)
body = [body_compressed]
else
# Remove content-encoding header for response with compressed content
headers.delete('Content-Encoding')
end
end
end

Expand Down
19 changes: 10 additions & 9 deletions lib/response_bank/response_cache_handler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,15 @@ def cache_key_hash
private

def hash(key)
"cacheable:#{Digest::MD5.hexdigest(key)}"
"cacheable:" + Digest::MD5.hexdigest(key)
end

def entity_tag
@entity_tag ||= ResponseBank.cache_key_for(key: @key_data, version: @version_data, key_schema_version: @key_schema_version)
end

def cache_key
@cache_key ||= ResponseBank.cache_key_for(key: @key_data, key_schema_version: @key_schema_version)
@cache_key ||= ResponseBank.cache_key_for(key: @key_data, key_schema_version: @key_schema_version, encoding: @env['response_bank.server_cache_encoding'])
end

def cacheable_info_dump
Expand Down Expand Up @@ -143,16 +143,17 @@ def serve_from_cache(cache_key_hash, match_entity_tag = "*", cache_age_tolerance
# version check
# unversioned but tolerance threshold
# regen
@headers = @headers.merge(headers)
@headers.merge!(headers)

if @env["gzip"]
@headers['Content-Encoding'] = "gzip"
else
# we have to uncompress because the client doesn't support gzip
ResponseBank.log("uncompressing for client without gzip")
body = ResponseBank.decompress(body)
# if a cache key hit and client doesn't match encoding, return the raw body
if !@env['HTTP_ACCEPT_ENCODING'].to_s.include?(@headers['Content-Encoding'])
ResponseBank.log("uncompressing payload for client as client doesn't require encoding")
body = ResponseBank.decompress(body, @headers['Content-Encoding'])
@headers.delete('Content-Encoding')
end

[status, @headers, [body]]

end
end

Expand Down
2 changes: 1 addition & 1 deletion lib/response_bank/version.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# frozen_string_literal: true
module ResponseBank
VERSION = "1.2.2"
VERSION = "1.3.0"
end
2 changes: 2 additions & 0 deletions response_bank.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@ Gem::Specification.new do |s|
s.metadata["allowed_push_host"] = "https://rubygems.org"

s.add_runtime_dependency("msgpack")
s.add_runtime_dependency("brotli")

s.add_development_dependency("minitest", ">= 5.18.0")
s.add_development_dependency("mocha", ">= 2.0.0")
s.add_development_dependency("rake")
s.add_development_dependency("rails", ">= 6.1")
s.add_development_dependency("pry")
end
5 changes: 3 additions & 2 deletions test/controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def test_cache_control_no_store_set_for_uncacheable_requests
end

def test_server_cache_hit
controller.request.env['gzip'] = false
controller.request.env['response_bank.server_cache_encoding'] = 'br'
@cache_store.expects(:read).returns(page_serialized)
ResponseBank::ResponseCacheHandler.any_instance.expects(:entity_tag_hash).returns('*').at_least_once
controller.expects(:render).with(plain: '<body>hi.</body>', status: 200)
Expand All @@ -64,6 +64,7 @@ def test_server_cache_hit
end

def test_client_cache_hit
controller.request.env['response_bank.server_cache_encoding'] = 'br'
controller.request.env['HTTP_IF_NONE_MATCH'] = 'deadbeef'
ResponseBank::ResponseCacheHandler.any_instance.expects(:entity_tag_hash).returns('deadbeef').at_least_once
controller.expects(:head).with(:not_modified)
Expand All @@ -78,6 +79,6 @@ def controller
end

def page_serialized
MessagePack.dump([200, {"Content-Type" => "text/html"}, ResponseBank.compress("<body>hi.</body>"), 1331765506])
MessagePack.dump([200, {"Content-Type" => "text/html", "Content-Encoding" => "br"}, ResponseBank.compress("<body>hi.</body>", "br"), 1331765506])
end
end
Loading