-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
HTTP/2 push support plugin (golang 1.8) #1215
Conversation
Wow, fantastic @wendigo! I haven't looked at this yet (just woke up) but I am really surprised -- wasn't expecting to see this show up in my inbox. 😄 This definitely belongs in core, not a separate repo. I have had some thoughts on server push and how Caddy should/might handle it -- let me elaborate them here briefly just to give you an idea.
In any case, I like the idea of the user being able manually specify push rules and using the Link header, at least to start. But I'm really intent on making server push automatic in the future. What do you think? Thanks to build tags, this PR is safe to use pre-Go 1.8 (nice work). I'm willing to accept this PR with the proposed set of functionality (without the "automatic push" features) -- but I and other reviewers should go over it in more detail first of course. Expect some iterations on this before it's ready to go. Thanks, I'm excited for this! |
I don't know if parsing HTML body is a good idea - it won't catch most of the resources (like images, fonts referenced in CSS files, javascripts/images loaded by javascripts and so on). Maybe supporting something like push manifest is a good idea? It can be dynamically reloaded / watched via fsnotify and constructed by any tool you want (using machine learning based on logs). Also pushing whole directories might be a good idea, i.e. push everything in css/ and img/ folders. |
I've added header/method validation and loaded plugin. I've built caddy both on 1.8 (current tip) and 1.7.3 and it does not break Caddy. |
Fix for building on 1.8: https://github.com/mholt/custombuild/pull/8/files |
I've also fixed the case when using:
will cause recursive push (requested /index.css will match rule and try to push /index.css and so on). |
Updated H2 bundle landed in golang's tip (golang/go@07e7266) so this change can be easily tested from now on (and functional on tip 👍) |
I'll need some time before I can review this thoroughly -- any other collaborators are welcome and invited to review in the meantime! (Don't merge yet though. I don't normally say that, but this is a pretty big change.) |
Hello @wendigo, we want to implement X25519 and CHACHA20-POLY1305 😄 There will be no need to use P.S.: I will review your code in the next weeks 😄, if I have enough time. |
@elcore: I know, I just wanted this change to not break current go version 1.7 - so it can be merged without breaking anything. After 1.8 release build tags can be removed. |
@wendigo perfect 😄 |
I am keeping my eye on this, just so you know ;) |
caddyhttp/push/handler_go18.go
Outdated
pusher, hasPusher := w.(http.Pusher) | ||
|
||
// No Pusher, no cry | ||
if hasPusher { |
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.
You can get rid of a lot of nesting with
if !hasPusher {
return h.Next.ServeHTTP(w, r)
}
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.
You're right - thx :) Fixed!
aa3adac
to
90b8090
Compare
I only partly agree. 😉 We can parse CSS bodies too. JavaScript, a little harder; I wouldn't worry about JS for now. I bet parsing HTML and CSS files would get us most of the way there without much difficulty. |
The machine "learning" could be pretty dumb and still work. What if one-in-N requests was served without using push, and then you noted which subsequent requests were serving files with a mime-type of CSS/JS/image? |
Yes, indeed. It would basically look like a map of request path to a list of subsequent request paths. (There would be some more data attached to each path in a struct, like validity period or eviction time or something like that... in order to keep the map updated over the lifetime of the server.) So those are the two approaches I'm deciding between to make push automatic: parsing HTML/CSS documents is one way, and the other way is observing request patterns over time. The first works instantly and is fairly mechanical, straightforward, but may miss some pushes. The second takes a little time to get optimal, but should work well in that it won't "miss" anything it couldn't parse. |
Something which would be nice is if a backend server sends a preload header e.g. using cloudflare/netjet to automatically push these resources too |
@lenovouser |
Ah, okay. So that means Caddy sees the |
Ah, yeah. Seems like it. Sorry - totally missed that. |
@lenovouser exactly, Caddy with push plugin will intercept |
@tdewolff just made https://github.com/tdewolff/push which does the automatic push based on parsing contents. If we can find a way to cache the parse results so the parsing doesn't have to happen on every request, I think we should be able to add this to Caddy. (But it doesn't need to be a part of this PR.) |
Looking into it @mholt :) |
Is this relevant https://github.com/tdewolff/push? |
@brasilikum Yes, see my comment two above yours 😉 |
Regarding how to use server push, it turns out it's hard. These links might be of interest: |
fcc4c82
to
e9885ba
Compare
@wendigo This should help get things started, take a look at this issue: tdewolff/push#1 Basically, we should be caching (for some configurable amount of time) the results of parsing the resources so that we don't have to do it on every single request. It would be interesting to first implement it very simply without caching, and just do the bare-bones auto-push, and then benchmark it using wrk or something. Then after that, implement the caching and see how it performs. Getting it set up for the first benchmarks should only be a few lines of code then. Does that make sense? |
Should this feature implicitly copy certain request headers into the the |
@nwidger Do you mean the auto-push (TODO) or the manually-configured push? I don't believe the Accept-Encoding header, which is on the request, needs to be pushed into the response anyway, though. |
But a push response goes down to the client, not to a backend. |
Other important headers to copy might include |
@mholt The |
Ah, I see what you mean. Yes... perhaps, see what the proxy middleware sends upstream to get a decent list. |
If I'm reading the code right, it looks like Apache's mod_http2 implementation copies the |
Similarly, nghttp2 appears to copy the |
@nwidger thx, working on it right now |
return h.Next.ServeHTTP(w, r) | ||
} | ||
|
||
// Serve file first |
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 description for http.Pusher contains the following text:
// Handlers that wish to push URL X should call Push before sending any
// data that may trigger a request for URL X. This avoids a race where the
// client issues requests for X before receiving the PUSH_PROMISE for X.
By serving the original request first, aren't we violating this rule? The client will receive the Link headers and may send out additional requests for those resources before we send out the push promises for them.
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 didn't see that comment - if it's a rule - it should not be violated :) Fixed in #1453
Hi, Just tried http2 push but its not working: my.domain:443 { But no Link headers with pushed resources while GETting / and them are not pushed. |
2017/04/12 21:15:30 Caddyfile:7 - Parse error: Unknown directive 'push' Caddy 0.9.5 I may be something wrong? |
use last code from git. Push is known directive but it not working at all. BTW, not sure but you cant use push with other domain then yours. you should push resources from www.lishengcn.cn and not img. |
I installed caddy by not img? you mean only javascript and css? |
@lishengzxc That's the latest release but the version that has the push feature is only build-from-source for now. |
@mholt Thx, I will try |
Hi all,
This change introduces new
push
plugin (adressing #816)This will be possible in golang 1.8 due to
http.Pusher
andhttp.PushOptions
interfaces being introduced (golang/go@cf73bbf, tracking golang/go#13443)As interface was published yesterday I could not resist and implemented new
push
directive 😁This middleware operates in two modes: rules-based and link-based.
Rules-based syntax
Link-based mode
Middleware intercepts
Link
headers from theNext
middleware and tries to usePusher
to push resources to the client.For more information about
Link
header: https://w3c.github.io/preload/Note for reviewers
This plugin is complete and tested but non functional as of current go's tip (actual implementation in https://go-review.googlesource.com/#/c/29439/ was not yet merged).
But... when go 1.8 will be released this plugin will actually support HTTP/2 Push in Caddy
Discussion
I'm not sure if this plugin should be integral part of the caddy's core or should I move it to it's own repo - opinions on that will be appreciated.
https://www.youtube.com/watch?v=vCadcBR95oU 🕶