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 H2 early hints #2047

Open
PikachuEXE opened this issue Mar 20, 2018 · 10 comments
Open

Support H2 early hints #2047

PikachuEXE opened this issue Mar 20, 2018 · 10 comments

Comments

@PikachuEXE
Copy link
Contributor

Issue report

Fill in as much as possible so that we can understand, find and fix the problem.

Question 1: What is the problem?

  • What is the expected behavior?
    Support setting rack.early_hints in env when processing a request which allow rails to send early hint headers
  • What is the actual behavior?
    Not supported yet

Rails PR
Puma PR

Question 2: Passenger version and integration mode:
open source 5.2.1 standalone

Your answer:

Question 3: OS or Linux distro, platform (including version):
OS X 10.12.6 Sierra, x86_64

Your answer:

Question 4: Passenger installation method:

Your answer:
[x] RubyGems + Gemfile
[ ] RubyGems, no Gemfile
[ ] Phusion APT repo
[ ] Phusion YUM repo
[ ] OS X Homebrew
[ ] source tarball
[ ] Other, please specify:

Question 5: Your app's programming language (including any version managers) and framework (including versions):

Your answer:
Ruby 2.5.0, RVM, Rails 5.1.5

@PikachuEXE
Copy link
Contributor Author

I try to update the Server side code myself but I have no idea how to test C/C++ code locally

stable-5.3...PikachuEXE:feature/h2-early-hints

@FooBarWidget
Copy link
Member

Thanks for this suggestion and contribution. We'll take a closer look in the near future.

@FooBarWidget FooBarWidget added this to the 5.3.0 milestone Apr 4, 2018
@CamJN
Copy link
Contributor

CamJN commented May 1, 2018

For testing I did this:

compiled the branch with rake nginx
create a new rails 5.2 app with a welcome#index view as the root path.
removed puma from the Gemfile & bundle installed to update lock file
started the app with /path/to/passenger-src/bin/passenger start --ssl --ssl-certificate-key /path/to/server.key --ssl-certificate /path/to/server.crt --log-level 7
made a request with curl -k --http2 https://localhost:3000/

Currently it appears that there is a problem with ForwardResponse.cpp, where the request is never completed.

@CamJN
Copy link
Contributor

CamJN commented May 3, 2018

This currently sends as many 103 responses as there are asset tags, which MAY be valid but certainly isn't ideal. This is due to rails calling the rack EARLY HINTS lambda in each asset tag (https://github.com/rails/rails/blob/master/actionview/lib/action_view/helpers/asset_tag_helper.rb).

I can't seem to find the rack spec for early hints, to check if the callback should be called only once or if multiple calls are permitted and the app server should aggregate on its end. Since rails is going for n calls, we'll likely have to support that regardless.

@PikachuEXE
Copy link
Contributor Author

I guess it's not very possible to have 1 call only
Since rails implement in this way:

===== Hi I am splitter =====

  1. Get a request
  2. Start generating response
  3. Scan template, execute helper functions
  4. When calling script tag / stylesheet tag, call a method which emits 103 to response if supported and enabled (via rack.early_hints proc)

If it's not implemented that way the hints won't be "early"

@CamJN
Copy link
Contributor

CamJN commented May 7, 2018

I've got this working in the standalone builtin engine, but both nginx and apache have issues with the early-header style.

@PikachuEXE
Copy link
Contributor Author

@CamJN
Care to share the issues you encountered?
If I understand how passenger C/C++ code is structured I can submit a PR myself
But I can't :/

@FooBarWidget
Copy link
Member

I think Camden was referring to issues within Nginx and Apache itself. Factors that we have little control over.

By the way w.r.t. how the Passenger C++ code is structured: I've been playing with the idea for a while of slowly moving portions of the Passenger codebase to Go, in order to make development and maintenance easier in the future. The biggest problem would be requiring users who aren't using our binaries (e.g. Debian packages) to have a sufficiently recent Go compiler installed. What do you think of this idea?

@PikachuEXE
Copy link
Contributor Author

It depends on how easy to get and use the compiler for developing passenger in different environments
Also we might need some document to provide an overview on code structure
Right now I can't find any so I am just guessing from reading the code

@FooBarWidget
Copy link
Member

Yes I agree, I am also thinking about writing more developer documentation in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants