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
Add early hints feature #1403
Add early hints feature #1403
Conversation
e2f4042
to
59764b6
Compare
@eileencodes and I are working on pushing early hints support in to the Rack spec for version 2 (version 2 of the SPEC, not version 2 of the gem). I'd like to try experimentally implementing this in Puma first, then push support to Rails, then upstream to the Rack SPEC. IOW, I don't want this to be done in a vacuum. Does this seem like an acceptable approach for the Puma team? I can understand if you don't want to be used for experimentation, but I would like to get this in to production systems sooner rather than later. 😊 Also, we've verified this to work with Puma + h2o. |
I think we're using Ruby 2.3+ specific features in the test in this PR. If the overall concept seems OK, then we'll fix it up. |
👏 Give me a while to look at it, headed to Japan soon so not sure how much time I'll have. Last resort we can talk about it at Kaigi? |
@nateberkopec sounds good. 😊 |
also I think I speak for the whole Puma team when I say that we have no problem with experimentation. hell Evan wanted to add native websockets and rewrite the entire reactor. so adding an optional feature...pretty tame 😆 |
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.
Couple of things just for "Puma style" concerns.
lib/puma/server.rb
Outdated
fast_write client, "\r\n".freeze | ||
} | ||
else | ||
env[EARLY_HINTS] = lambda { |_| } |
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.
Why is this necessary? I guess I don't understand.
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 is so that app code can run without changes even if the server is being run without early hints support. The example app that @eileencodes put in the PR description can be run whether early hints is enabled or not (it prevents the app code from writing a if env[EARLY_HINTS]
conditional)
lib/puma/server.rb
Outdated
@@ -595,6 +600,24 @@ def handle_request(req, lines) | |||
env[RACK_INPUT] = body | |||
env[RACK_URL_SCHEME] = env[HTTPS_KEY] ? HTTPS : HTTP | |||
|
|||
if @early_hints | |||
env[EARLY_HINTS] = lambda { |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.
It would be nice to have this lambda live somewhere other than the body of this method.
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.
Agree. @eileencodes and I were trying to move it out of here, but the lambda body needs to access the io object (client
) and fast_write
is a private method. I'd love to move the lambda out of here, but I am not sure how.
lib/puma/dsl.rb
Outdated
@@ -241,6 +241,10 @@ def tcp_mode! | |||
@options[:mode] = :tcp | |||
end | |||
|
|||
def 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.
I know our DSL style is really inconsistent, but for futureproofing I'd prefer:
def early_hints(answer=true)
@options[:early_hints] = answer
end
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 👍
lib/puma/server.rb
Outdated
links.each do |link| | ||
fast_write client, "Link: <#{link[:link]}>; rel=preload" | ||
if as = link[:as] | ||
fast_write client, "; as=#{as}" |
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.
Should we auto-detect this part? I did some of this in rack-http-preload..
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.
Unsure. h2o pushes the resource regardless. I think this is something we need to iterate on. (IOW, I don't have an answer)
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 also possible we may not want to auto-detect as
at the puma level. I just wanted to ask the question.
lib/puma/server.rb
Outdated
@@ -97,6 +98,10 @@ def tcp_mode! | |||
@mode = :tcp | |||
end | |||
|
|||
def 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.
I think you were copying tcp_mode!, but that's a !
method because it doesn't make sense for a server to be in tcp_mode and then not be in tcp mode later. Although I can't think of why anyone ever would (restarting and disabling early hints as the config changes?), it's more plausible that a server could toggle its support of early_hints. So this should probably just be treated like a straight-up accessor.
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.
Moved to an attr_accessor
and deleted this method.
test/test_puma_server.rb
Outdated
|
||
data = sock.read | ||
|
||
assert_equal <<~eos.split("\n").join("\r\n") + "\r\n\r\n", data |
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.
Rubocop is barfing on your squiggly heredoc.
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 saw the error, and I think RuboCop needs to be fixed. This is new syntax in Ruby 2.3. But also, since this is new syntax in Ruby 2.3, and you probably want to run the Puma tests on older versions of Ruby anyway, we'll fix it. 😉
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 the tests here too. 👍 I also added a test to test that early hints is off by default.
/cc @rafaelfranca |
67b2347
to
d444ed3
Compare
Need to look at this one more time when I'm not jetlagged but I think this is ready. |
c55bf5c
to
4b87272
Compare
Hey folks. I've been talking to @matthewd about this feature. He convinced me that the lambda should take a header hash just like the header hash from a rack response. This would alleviate webservers from knowing how the IOW, we should change the API to be something like this:
WDYT? |
@tenderlove If I'm reading the HTTP 103 spec correctly:
So, 103 status CANNOT contain a Connection, Content-Length or Transfer-Encoding header, but MAY contain any other header? If I'm getting that right, your change makes sense. But should anyone be responsible for omitting the prohibited headers? Puma's job or someone elses? |
I think if you wanted to omit them in Puma it would be fine. However, I'd expect the proxy to complain or raise an error if it got those headers, so maybe Puma shouldn't care? It would be nice for the user to get an exception closer to where the error is occurring, but I don't think it's necessary. |
This commit adds the early hints lambda to the headers hash. Users can call it to emit the early hints headers. For example: ``` class Server def call env if env["REQUEST_PATH"] == "/" env['rack.early_hints'].call("Link" => "</style.css>; rel=preload; as=style\n</script.js>; rel=preload") [200, { "X-Hello" => "World" }, ["Hello world!"]] else [200, { "X-Hello" => "World" }, ["NEAT!"]] end end end run Server.new ``` In this example, the server sends stylesheet and javascript early hints if the proxy supports it, it will send H2 pushes to the client. Of course not every proxy server supports early hints, so to enable the early hints feature with puma you have to pass the configuration variable, `--early-hints`. If `ENV['rack.early_hints']` is not set then early hints is not supported by the webserver. Early hints is off by default.
4b87272
to
ea37ada
Compare
I've updated the PR so that now the lambda now takes a header hash and tested this in the Rails implementation for our test app. Let me know if you'd like other changes but I think this is good to go now. |
fast_write client, "#{k}: #{v}\r\n" | ||
end | ||
else | ||
fast_write client, "#{k}: #{v}\r\n" |
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.
Should this be fast_write client, "#{k}: #{vs}\r\n"
instead?
I don't see v
defined in this level
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.
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.
Oh i see what you're saying - I will fix it
This is similar to puma/puma#1403
This is similar to puma/puma#1403
Was this ever integrated into the Rack SPEC? I had a look but couldn't see anything on the latest master branch. What was the final design of the API? I'm implementing this in |
I implemented the design that was merged into puma. Here is my brief feedback:
In summary: is this interface too generic? Would it be better to limit it to push-promise functionality, or is it good to have it allow any header? |
I wrote up a brief summary regarding implementing this feature. https://www.codeotaku.com/journal/2019-02/falcon-early-hints/index |
Worked with @tenderlove on adding early hints. This is still a work in progress as there are a few things we need to address, but this PR is to start the conversation.
This commit adds the early hints lambda to the headers hash. Users can
call it to emit the early hints headers. For example:
In this example, the server sends stylesheet and javascript early hints
if the proxy supports it, it will send H2 pushes to the client.
Of course not every proxy server supports early hints, so to enable the
early hints feature with puma you have to pass the configuration variable,
--early-hints
.