-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Implement H2 Early Hints for Rails #30744
Conversation
0528011
to
063d77b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc, sprockets-rails overrides these helpers, but that won't make it so that the early hints will be removed, right?
Looking great otherwise!
@@ -199,6 +199,23 @@ def headers | |||
@headers ||= Http::Headers.new(self) | |||
end | |||
|
|||
# Early Hints is an H2 status code that indicates hints to help a client start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is H2 an accepted short hand for HTTP2? Otherwise we should probably stick to HTTP2 through out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it is an accepted shorthand I've replaced this with HTTP/2 because documentation should not assume knowledge of that.
# | ||
# The +send_early_hints+ method accepts an array of links as follows: | ||
# | ||
# send_early_hints({ link: "path/to/stylesheet.css", as: "stylesheet}, { link: "path/to/javascript.js", as: "script"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a closing quote on as: "stylesheet
. Should add a space before the two }
as well.
assert_equal early_hints.first[:link], "a_path.css" | ||
assert_equal early_hints.first[:as], "stylesheet" | ||
assert_equal early_hints.last[:link], "a_path.js" | ||
assert_equal early_hints.last[:as], "script" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument order here should be reversed. It's expected, then actual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed when the API was updated.
@@ -37,6 +37,9 @@ module AssetTagHelper | |||
# When the Asset Pipeline is enabled, you can pass the name of your manifest as | |||
# source, and include other JavaScript or CoffeeScript files inside the manifest. | |||
# | |||
# If the server supports Early Hints header links for these assets will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double space after assets
.
}.merge!(options) | ||
content_tag("script".freeze, "", tag_options) | ||
}.join("\n").html_safe | ||
|
||
request.send_early_hints(early_hints_links) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads to me as if users can only call javascript_include_tag
once, is that right?
It's possible people have calls in their application layout and then in individual template files (I'm not saying that's a good idea, but I think that does exist).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote it this way so that if you pass multiple sources into javascript_include_tag
that it will only send early hints once, but you can call javascript_include_tag
as many times as you want and it will send early hints.
tag_options = { | ||
"src" => path_to_javascript(source, path_options) | ||
"src" => href | ||
}.merge!(options) | ||
content_tag("script".freeze, "", tag_options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to change this to tag.script(src: href, **options)
while we're here? (Though that would probably necessitate shifting to symbolize_keys
above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that desirable in general in internal stuff? The improvement for user code is nice, but it feels like fairly pointless (and comparatively allocation-heavy) indirection for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it's a bit of a back and forth. I was focusing on removing the freeze
call. Let's defer to some other time, since it's out of scope for this particular thing 👍
def send_early_hints(links) | ||
return unless env["rack.early_hints"] | ||
|
||
env["rack.early_hints"].call(links) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could avoid calling doing the same lookup twice:
env["rack.early_hints"]&.call(links)
or
callback = env["rack.early_hints"]
callback.call(links) if callback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a hash lookup. I'm sure it'll be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've considered this but agree with @matthewd.
Drying things up isn't a benefit if the code is harder to read and understand and the performance impact of calling a hash lookup twice will be negligible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just curious about the performance impact, so I did a quick benchmarking.
TL;DR
... the performance impact of calling a hash lookup twice will be negligible.
Details
# 1.18x slower than v2 if env["rack.early_hints"]
def v1
return unless env["rack.early_hints"]
env["rack.early_hints"].call(links)
end
# 1.09x slower than v1 IF NOT env["rack.early_hints"]
def v2
callback = env["rack.early_hints"]
callback.call(links) if callback
end
Click to show code
#!/usr/bin/env ruby
require 'benchmark/ips'
require 'date'
LOOKUP = { "foo" => ->() {} }
def happy_case
return unless LOOKUP["foo"]
LOOKUP["foo"].call()
end
def happy_case_with_assignment
callback = LOOKUP["foo"]
callback.call() if callback
end
def alternase_case
return unless LOOKUP["bar"]
LOOKUP["bar"].call()
end
def alternase_case_with_assignment
callback = LOOKUP["bar"]
callback.call() if callback
end
# == Result
#
# happy_case: 1.18x slower
Benchmark.ips do |x|
# Configure the number of seconds used during
# the warmup phase (default 2) and calculation phase (default 5)
x.config(time: 5, warmup: 2)
x.report("happy_case") { happy_case }
x.report("happy_case_with_assignment") { happy_case_with_assignment }
# Compare the iterations per second of the various reports!
x.compare!
end
# == Result
#
# alternase_case_with_assignment: 1.09x slower
Benchmark.ips do |x|
# Configure the number of seconds used during
# the warmup phase (default 2) and calculation phase (default 5)
x.config(time: 5, warmup: 2)
x.report("alternase_case") { alternase_case }
x.report("alternase_case_with_assignment") { alternase_case_with_assignment }
# Compare the iterations per second of the various reports!
x.compare!
end
__END__
Calculating -------------------------------------
happy_case 111.305k i/100ms
happy_case_with_assignment
127.680k i/100ms
-------------------------------------------------
happy_case 3.704M (± 2.1%) i/s - 18.588M
happy_case_with_assignment
4.353M (± 1.2%) i/s - 21.833M
Comparison:
happy_case_with_assignment: 4353466.9 i/s
happy_case: 3704451.9 i/s - 1.18x slower
Calculating -------------------------------------
alternase_case 146.388k i/100ms
alternase_case_with_assignment
154.871k i/100ms
-------------------------------------------------
alternase_case 7.631M (± 2.9%) i/s - 38.207M
alternase_case_with_assignment
6.977M (± 6.8%) i/s - 34.846M
Comparison:
alternase_case: 7630584.3 i/s
alternase_case_with_assignment: 6977239.4 i/s - 1.09x slower
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, would have been nice to see how the &.
syntax compares.
@@ -161,7 +162,8 @@ def server_options | |||
daemonize: options[:daemon], | |||
pid: pid, | |||
caching: options["dev-caching"], | |||
restart_cmd: restart_command | |||
restart_cmd: restart_command, | |||
early_hints: early_hints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use it directly like dev-caching / daemon ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put the documentation in the early hints option. It's passed in like --early-hints
or can be set in the puma.config with early_hints(true)
.
just a bad pattern in general and not dry ... so prefer to avoid even if
it's fine here
…On Fri, Sep 29, 2017 at 12:24 AM, Matthew Draper ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In actionpack/lib/action_dispatch/http/request.rb
<#30744 (comment)>:
> @@ -199,6 +199,23 @@ def headers
@headers ||= Http::Headers.new(self)
end
+ # Early Hints is an H2 status code that indicates hints to help a client start
+ # making preparations for processing the final response.
+ #
+ # If the env contains +rack.early_hints+ then the server accepts HTTP2 push for Link headers.
+ #
+ # The +send_early_hints+ method accepts an array of links as follows:
+ #
+ # send_early_hints({ link: "path/to/stylesheet.css", as: "stylesheet}, { link: "path/to/javascript.js", as: "script"})
+ #
+ # If you are using +javascript_include_tag+ or +stylesheet_link_tag+ the
+ # Early Hints headers are included by default if supported.
+ def send_early_hints(links)
+ return unless env["rack.early_hints"]
+
+ env["rack.early_hints"].call(links)
It's a hash lookup. I'm sure it'll be fine.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#30744 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZ0RzsToBpMY-tEMsTZq-csqYRJ5Vks5snJsxgaJpZM4PnuP3>
.
|
063d77b
to
9ee7c4a
Compare
@kaspth I've fixed the typos and other recommendations you made. Thanks for the review! FYI, this is on hold until the Puma branch is merged. |
9ee7c4a
to
64ce53b
Compare
When puma/puma#1403 is merged Puma will support the Early Hints status code for sending assets before a request has finished. While the Early Hints spec is still in draft, this PR prepares Rails to allowing this status code. If the proxy server supports Early Hints, it will send H2 pushes to the client. This PR adds a method for setting Early Hints Link headers via Rails, and also automatically sends Early Hints if supported from the `stylesheet_link_tag` and the `javascript_include_tag`. Once puma supports Early Hints the `--early-hints` argument can be passed to the server to enable this or set in the puma config with `early_hints(true)`. Note that for Early Hints to work in the browser the requirements are 1) a proxy that can handle H2, and 2) HTTPS. To start the server with Early Hints enabled pass `--early-hints` to `rails s`. This has been verified to work with h2o, Puma, and Rails with Chrome. The commit adds a new option to the rails server to enable early hints for Puma. Early Hints spec: https://tools.ietf.org/html/draft-ietf-httpbis-early-hints-04 [Eileen M. Uchitelle, Aaron Patterson]
64ce53b
to
59a02fb
Compare
@eileencodes What about javascript_pack_tag? It will also work with it? |
@madmax Is that implemented by webpacker? I haven't used Rails webpacker and I don't see a |
Yes it is part of webpacker gem. And there is also manifest.json can't we use it? |
At this moment I don't plan on implementing in webpacker but I don't see a reason to not implement it there if the tag has access to the |
IIRC |
Ok then I think it will Just Work™️ 😄 |
According to code, links for early hints are formatted even when server does not support early hints. Should this be better enabled with Also this should be a better option to provide method to send hints from controller, this way browser can start loading scripts even before server starts business logic and page rendering. Layout rendering is the last stage (except streaming mode) of request processing, hints can be even earlier (in before_action?). WDYT? |
|
||
sources_tags = sources.uniq.map { |source| | ||
href = path_to_stylesheet(source, path_options) | ||
early_hints_links << "<#{href}>; rel=preload; as=stylesheet" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the example described in https://w3c.github.io/preload/#as-attribute, it seems that the as
target attribute for stylesheets should be style
instead of stylesheet
Hey @eileencodes! Thanks so much for your work getting early hints supported in Rails. I'm seeing some intermittent socket timeout errors when I have --early-hints enabled on my app, which is hosted on Heroku. It's only showing up about once a day, and only in production. Some lucky visitor's request is throwing one Puma:ConnectionError for each Any hunches?
|
Hey @pjforde1978 I'm not sure what's going on but I suspect that this is more on the puma side than the Rails side since Rails just adds the tags, but Puma makes the early hints actually work. Can you open an issue over there including what your application is using (Ruby version, Rails version, Puma version, proxy you're using), and anything else that can help us track this down. Unfortunately we're not able to use this in prod at GitHub yet so I'm not sure where to start looking. |
Message for Eileen: Hi @eileencodes, thanks for adding this fantastic feature. I've been investigating the error reported by @pjforde1978. This happens if the connection is lost before we're able to send any response headers. Rails will try to write the early hints on a socket which no longer exists so it'll fail. This happens very rarely and probably no end-users are being affected as I'm assuming the connection is lost when the user abandons the page load. From Rails' perspective, there's nothing we can do if the socket has been closed. I think we can ignore this error safely. But do you think Rails should bother to abort completing this request if the early hints report a lost connection somehow? |
When puma/puma#1403 is merged Puma will support the Early Hints status
code for sending assets before a request has finished.
While the Early Hints spec is still in draft, this PR prepares Rails to
allowing this status code.
If the proxy server supports Early Hints, it will send H2 pushes to the
client.
This PR adds a method for setting Early Hints Link headers via Rails,
and also automatically sends Early Hints if supported from the
stylesheet_link_tag
and thejavascript_include_tag
.Once puma supports Early Hints the
--early-hints
argument can bepassed to the server to enable this. Note that for Early Hints to work
in the browser the requirements are 1) a proxy that can handle H2,
and 2) HTTPS.
To start the server with Early Hints enabled pass
--early-hints
torails s
.This has been verified to work with h2o, Puma, and Rails with Chrome.
The commit adds a new option to the rails server to enable early hints
for Puma.
Early Hints spec:
https://tools.ietf.org/html/draft-ietf-httpbis-early-hints-04
cc/ @tenderlove who I worked with on this feature
In the screenshot below you can see the pushes from early hints.