Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Implement NgxServerContext::ApplySessionFetchers #193

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
4 participants
Owner

oschaaf commented Mar 27, 2013

Makes sure we use the LoopbackRouteFetcher, and also wires up AddHeadersFetcher.

Note that this pulls in loopback_route_fetcher.cc from svn revision r2649

The LoopbackRouteFetcher is applied unconditionally, while in mod_pagespeed it is not applied when one of these configuration settings is true:

  • disable_loopback_routing
  • slurping_enabled, or
  • test_proxy is set

I added a TODO for that.

Contributor

jeffkaufman commented Mar 27, 2013

When we add .cc files to psol/include we need to list them in scripts/copy_includes.sh. By default we get all of the .h files and none of the .cc or .c files.

Otherwise the next time we update our binaries against trunk we'll have build issues.

Contributor

jeffkaufman commented Mar 27, 2013

This is a little scary because we're manually pulling in changes from a later version of mod_pagespeed than the r2618 we've been using. None of the changes are (I think!) reflected in the binaries we distribute, so it may work. People who build the "complex way" will get the version of loopback_route_fetcher.cc from r2618 and not r2649. Which I think will work, just not get the somewhat minor improvements of the interim changes. We should at least test that it works in both the "simple" and "complex" flows, though.

Contributor

jeffkaufman commented Mar 27, 2013

Aside from the two issues I've mentioned it looks good to me and passes the tests I've added in #194. I've asked @morlovich to have a look, however, as he wrote the relevant mod_pagespeed code.

@morlovich morlovich and 1 other commented on an outdated diff Mar 27, 2013

src/ngx_request_context.cc
+ // Note that at the time we create a RequestContext we have full
+ // access to the nginx internal request structure. However,
+ // due to Cloning and (I believe) Detaching, we can initiate fetches after
+ // http_http_request_t* has been retired. So deep-copy the bits we need
+ // at the time we create our RequestContext.
+
+ // Save our own IP as well, LoopbackRouteFetcher will need it.
+
+ // Based on ngx_http_variable_server_port.
+ ngx_http_request_t* req = ps_request_ctx->r;
+ bool port_set = false;
+#if (NGX_HAVE_INET6)
+ if (req->connection->local_sockaddr->sa_family == AF_INET6) {
+ local_port_ = ntohs(reinterpret_cast<struct sockaddr_in6*>(
+ req->connection->local_sockaddr)->sin6_port);
+ have_port= true;
@morlovich

morlovich Mar 27, 2013

Contributor

The code below seems to be checking port_set_ rather than have_port.

@morlovich morlovich and 1 other commented on an outdated diff Mar 27, 2013

src/ngx_request_context.h
+#include "net/instaweb/util/public/string.h"
+#include "net/instaweb/util/public/string_util.h"
+
+
+namespace net_instaweb {
+
+class AbstractMutex;
+
+class NgxRequestContext : public RequestContext {
+ public:
+ NgxRequestContext(AbstractMutex* logging_mutex,
+ ngx_psol::ps_request_ctx_t* ps_request_context);
+
+ // Captures the original URL of the request, which is used to help
+ // authorize domains for fetches we do on behalf of that request.
+ void set_url(StringPiece url) { url.CopyToString(&url_); }
@morlovich

morlovich Mar 27, 2013

Contributor

I am confused by this. I don't think you need this method (nor does a corresponding method in Apache have anything to do with authorization --- it's used for the mod_spdy_fetcher.

Owner

oschaaf commented Mar 27, 2013

@jeffkaufman: "When we add .cc files to psol/include we need to list them in scripts/copy_includes.sh"

done c944281

Contributor

jeffkaufman commented on c944281 Mar 27, 2013

Looks good!

Owner

oschaaf commented Mar 27, 2013

@jeffkaufman "This is a little scary because we're manually pulling in changes from a later version of mod_pagespeed than the r2618 we've been using."

The complex method fails to build, because the constructor of LoopbackRouteFetcher has changed...
Should I try to pull them in the same way as we did with pthread_shared_mem?

Contributor

jeffkaufman commented Mar 28, 2013

@oschaaf That sounds fine as a temporary fix. Then when I rebuild the binaries next time it can go away.

Owner

oschaaf commented on 863f9ef Mar 28, 2013

@jeffkaufman "That sounds fine as a temporary fix. Then when I rebuild the binaries next time it can go away."

done in this commit (863f9ef)

Owner

oschaaf replied Mar 28, 2013

@jeffkaufman
As far as I can tell, LoopbackRouteFetcher and AddHeadersFetcher are not in pagespeed_automatic.a;
I tested this with both the simple and complex method to make sure, and also searched pagespeed_automatic.a using

nm -gC pagespeed_automatic.a | grep LoopbackRouteFetcher
nm -gC pagespeed_automatic.a | grep AddHeadersFetcher
Contributor

jeffkaufman commented Mar 28, 2013

@morlovich says it's good; merging

@jeffkaufman jeffkaufman pushed a commit that referenced this pull request Mar 28, 2013

Jeff Kaufman loopback-routing: apply session fetchers
Makes sure we use the LoopbackRouteFetcher, and also wires up AddHeadersFetcher.

Note that this pulls in loopback_route_fetcher.cc from svn revision r2649

The LoopbackRouteFetcher is applied unconditionally, while in mod_pagespeed it
is not applied when one of these configuration settings is true:

* disable_loopback_routing
* slurping_enabled, or
* test_proxy is set

I added a TODO for that.

(Sqash-merge of Otto's #193.)
057038d
Contributor

jeffkaufman commented Mar 28, 2013

merged: 057038d

@jeffkaufman jeffkaufman deleted the oschaaf-loopback-route-fetcher branch Mar 28, 2013

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