Skip to content

Commit

Permalink
Swallow connection errors when sending early hints (#1822)
Browse files Browse the repository at this point in the history
* Swallow connection errors when sending early hints

I've been thinking on how Puma should behave when the early hints
can't be sent because the connection has been lost.

I first thought that it should behave the same way it does when the
connection is lost before main response is sent, I've seen that it
swallows those errors (https://git.io/fjVtg) so perhaps we should do
the same here.

There's one consideration though. Because the early hints are sent at
the very beginning of the app execution, we may want to abort this
request altogether and save resources by throwing an exception and
causing Rails to interrupt execution.

Finally I decided to overlook that case just because I believe it's
least surprising from the Rack app point of view. Also
I haven't seen any other example of the web server interrupting
the apps execution due to an error on the lower layer.

Fixes #1640.

* Adds a test

* Updates test as per @nateberkopec's suggestion
  • Loading branch information
Jesus authored and nateberkopec committed Aug 1, 2019
1 parent b0c56db commit 2deaf77
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 9 deletions.
22 changes: 13 additions & 9 deletions lib/puma/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -634,19 +634,23 @@ def handle_request(req, lines)

if @early_hints
env[EARLY_HINTS] = lambda { |headers|
fast_write client, "HTTP/1.1 103 Early Hints\r\n".freeze
begin
fast_write client, "HTTP/1.1 103 Early Hints\r\n".freeze

headers.each_pair do |k, vs|
if vs.respond_to?(:to_s) && !vs.to_s.empty?
vs.to_s.split(NEWLINE).each do |v|
fast_write client, "#{k}: #{v}\r\n"
headers.each_pair do |k, vs|
if vs.respond_to?(:to_s) && !vs.to_s.empty?
vs.to_s.split(NEWLINE).each do |v|
fast_write client, "#{k}: #{v}\r\n"
end
else
fast_write client, "#{k}: #{vs}\r\n"
end
else
fast_write client, "#{k}: #{vs}\r\n"
end
end

fast_write client, "\r\n".freeze
fast_write client, "\r\n".freeze
rescue ConnectionError
# noop, if we lost the socket we just won't send the early hints
end
}
end

Expand Down
26 changes: 26 additions & 0 deletions test/test_puma_server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,32 @@ def test_early_hints_works
assert_equal expected_data, data
end

def test_early_hints_are_ignored_if_connection_lost
app = proc { |env|
env['rack.early_hints'].call("Link" => "</script.js>; rel=preload")
[200, { "X-Hello" => "World" }, ["Hello world!"]]
}

events = Puma::Events.strings
server = Puma::Server.new app, events
def server.fast_write(*args)
raise Puma::ConnectionError
end
server.add_tcp_listener @host, @port
server.early_hints = true
server.run

# This request will cause the server to try and send early hints
sock = TCPSocket.new @host, server.connected_port
sock << "HEAD / HTTP/1.0\r\n\r\n"

# Give the server some time to try to write (and fail)
sleep 0.1

# Expect no errors in stderr
assert events.stderr.pos.zero?, "Server didn't swallow the connection error"
end

def test_early_hints_is_off_by_default
@server.app = proc { |env|
assert_nil env['rack.early_hints']
Expand Down

0 comments on commit 2deaf77

Please sign in to comment.