diff --git a/src/ngx_base_fetch.cc b/src/ngx_base_fetch.cc index cc33f1d35..2af2297f2 100644 --- a/src/ngx_base_fetch.cc +++ b/src/ngx_base_fetch.cc @@ -38,16 +38,18 @@ NgxEventConnection* NgxBaseFetch::event_connection = NULL; NgxBaseFetch::NgxBaseFetch(ngx_http_request_t* r, NgxServerContext* server_context, const RequestContextPtr& request_ctx, - PreserveCachingHeaders preserve_caching_headers) + PreserveCachingHeaders preserve_caching_headers, + NgxBaseFetchType base_fetch_type) : AsyncFetch(request_ctx), request_(r), server_context_(server_context), done_called_(false), last_buf_sent_(false), references_(2), - ipro_lookup_(false), + base_fetch_type_(base_fetch_type), preserve_caching_headers_(preserve_caching_headers), - detached_(false) { + detached_(false), + suppress_(false) { if (pthread_mutex_init(&mutex_, NULL)) CHECK(0); } @@ -69,16 +71,32 @@ void NgxBaseFetch::Terminate() { } } +const char* BaseFetchTypeToCStr(NgxBaseFetchType type) { + switch(type) { + case kPageSpeedResource: + return "ps resource"; + case kHtmlTransform: + return "html transform"; + case kAdminPage: + return "admin page"; + case kIproLookup: + return "ipro lookup"; + } + CHECK(false); + return "can't get here"; +} + void NgxBaseFetch::ReadCallback(const ps_event_data& data) { NgxBaseFetch* base_fetch = reinterpret_cast(data.sender); ngx_http_request_t* r = base_fetch->request(); bool detached = base_fetch->detached(); + const char* type = BaseFetchTypeToCStr(base_fetch->base_fetch_type_); int refcount = base_fetch->DecrementRefCount(); #if (NGX_DEBUG) ngx_log_error(NGX_LOG_DEBUG, ngx_cycle->log, 0, - "pagespeed [%p] event: %c. bf:%p - refcnt:%d - det: %c", r, - data.type, base_fetch, refcount, detached ? 'Y': 'N'); + "pagespeed [%p] event: %c. bf:%p (%s) - refcnt:%d - det: %c", r, + data.type, base_fetch, type, refcount, detached ? 'Y': 'N'); #endif // If we ended up destructing the base fetch, or the request context is @@ -100,7 +118,8 @@ void NgxBaseFetch::ReadCallback(const ps_event_data& data) { int rc; // If we are unlucky enough to have our connection finalized mid-ipro-lookup, // we must enter a different flow. Also see ps_in_place_check_header_filter(). - if (!ctx->base_fetch->ipro_lookup_ && r->connection->error) { + if ((ctx->base_fetch->base_fetch_type_ != kIproLookup) + && r->connection->error) { ngx_log_error(NGX_LOG_DEBUG, ngx_cycle->log, 0, "pagespeed [%p] request already finalized", r); rc = NGX_ERROR; @@ -188,6 +207,10 @@ ngx_int_t NgxBaseFetch::CollectHeaders(ngx_http_headers_out_t* headers_out) { } void NgxBaseFetch::RequestCollection(char type) { + if (suppress_) { + return; + } + // We must optimistically increment the refcount, and decrement it // when we conclude we failed. If we only increment on a successfull write, // there's a small chance that between writing and adding to the refcount @@ -203,19 +226,21 @@ void NgxBaseFetch::HandleHeadersComplete() { int status_code = response_headers()->status_code(); bool status_ok = (status_code != 0) && (status_code < 400); - if (!ipro_lookup_ || status_ok) { + if ((base_fetch_type_ != kIproLookup) || status_ok) { // If this is a 404 response we need to count it in the stats. if (response_headers()->status_code() == HttpStatus::kNotFound) { server_context_->rewrite_stats()->resource_404_count()->Add(1); } } + RequestCollection(kHeadersComplete); // Headers available. + // For the IPRO lookup, supress notification of the nginx side here. - // If we send both this event and the one from done, nasty stuff will happen - // if we loose the race with with the nginx side destructing this base fetch - // instance (and thereby clearing the byte and its pending extraneous event). - if (!ipro_lookup_) { - RequestCollection(kHeadersComplete); // Headers available. + // If we send both the headerscomplete event and the one from done, nasty + // stuff will happen if we loose the race with with the nginx side destructing + // this base fetch instance. + if (base_fetch_type_ == kIproLookup && !status_ok) { + suppress_ = true; } } diff --git a/src/ngx_base_fetch.h b/src/ngx_base_fetch.h index 89325e5dc..6a3fa9cbb 100644 --- a/src/ngx_base_fetch.h +++ b/src/ngx_base_fetch.h @@ -64,12 +64,19 @@ extern "C" { namespace net_instaweb { +enum NgxBaseFetchType { + kIproLookup, + kHtmlTransform, + kPageSpeedResource, + kAdminPage +}; class NgxBaseFetch : public AsyncFetch { public: NgxBaseFetch(ngx_http_request_t* r, NgxServerContext* server_context, const RequestContextPtr& request_ctx, - PreserveCachingHeaders preserve_caching_headers); + PreserveCachingHeaders preserve_caching_headers, + NgxBaseFetchType base_fetch_type); virtual ~NgxBaseFetch(); // Statically initializes event_connection, require for PSOL and nginx to // communicate. @@ -100,8 +107,6 @@ class NgxBaseFetch : public AsyncFetch { // Called by pagespeed to increment the refcount. int IncrementRefCount(); - void set_ipro_lookup(bool x) { ipro_lookup_ = x; } - // Detach() is called when the nginx side releases this base fetch. It // sets detached_ to true and decrements the refcount. We need to know // this to be able to handle events which nginx request context has been @@ -151,10 +156,11 @@ class NgxBaseFetch : public AsyncFetch { // decremented on the nginx side for each event read for it. int references_; pthread_mutex_t mutex_; - bool ipro_lookup_; + NgxBaseFetchType base_fetch_type_; PreserveCachingHeaders preserve_caching_headers_; // Set to true just before the nginx side releases its reference bool detached_; + bool suppress_; DISALLOW_COPY_AND_ASSIGN(NgxBaseFetch); }; diff --git a/src/ngx_pagespeed.cc b/src/ngx_pagespeed.cc index 6d776df0e..594ca41bb 100644 --- a/src/ngx_pagespeed.cc +++ b/src/ngx_pagespeed.cc @@ -615,7 +615,6 @@ typedef struct { namespace RequestRouting { enum Response { kError, - kNotUnderstood, kStaticContent, kInvalidUrl, kPagespeedDisabled, @@ -628,7 +627,6 @@ enum Response { kCachePurge, kGlobalAdmin, kPagespeedSubrequest, - kNotHeadOrGet, kErrorResponse, kResource, }; @@ -1500,8 +1498,12 @@ void ps_release_base_fetch(ps_request_ctx_t* ctx) { } // TODO(chaizhenhua): merge into NgxBaseFetch ctor -ngx_int_t ps_create_base_fetch(ps_request_ctx_t* ctx, - RequestContextPtr request_context) { +void ps_create_base_fetch(ps_request_ctx_t* ctx, + RequestContextPtr request_context, + RequestHeaders* request_headers, + NgxBaseFetchType type) { + CHECK(ctx->base_fetch == NULL) << "Pre-existing base fetch!"; + ngx_http_request_t* r = ctx->r; ps_srv_conf_t* cfg_s = ps_get_srv_config(r); @@ -1509,11 +1511,10 @@ ngx_int_t ps_create_base_fetch(ps_request_ctx_t* ctx, // it, and call Done() on the associated parent (Proxy or Resource) fetch. If // we fail before creating the associated fetch then we need to call Done() on // the BaseFetch ourselves. - ctx->base_fetch = new NgxBaseFetch( - r, cfg_s->server_context, - request_context, ctx->preserve_caching_headers); - - return NGX_OK; + ctx->base_fetch = new NgxBaseFetch(r, cfg_s->server_context, + request_context, + ctx->preserve_caching_headers, type); + ctx->base_fetch->SetRequestHeadersTakingOwnership(request_headers); } void ps_release_request_context(void* data) { @@ -1754,35 +1755,19 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r, ngx_http_set_ctx(r, ctx, ngx_pagespeed); } - if (ps_create_base_fetch(ctx, request_context) != NGX_OK) { - // Do not need to release request context 'ctx'. - // http_pool_cleanup will call ps_release_request_context - return NGX_ERROR; - } - - ctx->base_fetch->SetRequestHeadersTakingOwnership(request_headers.release()); - - bool page_callback_added = false; - scoped_ptr - property_callback( - ProxyFetchFactory::InitiatePropertyCacheLookup( - !html_rewrite /* is_resource_fetch */, - url, - cfg_s->server_context, - options, - ctx->base_fetch, - false /* requires_blink_cohort (no longer unused) */, - &page_callback_added)); - if (pagespeed_resource) { // TODO(jefftk): Set using_spdy appropriately. See // ProxyInterface::ProxyRequestCallback + ps_create_base_fetch(ctx, request_context, request_headers.release(), + kPageSpeedResource); ResourceFetch::Start( url, custom_options.release() /* null if there aren't custom options */, false /* using_spdy */, cfg_s->server_context, ctx->base_fetch); return ps_async_wait_response(r); } else if (is_an_admin_handler) { + ps_create_base_fetch(ctx, request_context, request_headers.release(), + kAdminPage); QueryParams query_params; query_params.ParseFromUrl(url); @@ -1826,6 +1811,8 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r, } if (html_rewrite) { + ps_create_base_fetch(ctx, request_context, request_headers.release(), + kHtmlTransform); // Do not store driver in request_context, it's not safe. RewriteDriver* driver; @@ -1852,12 +1839,22 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r, driver->set_pagespeed_option_cookies(pagespeed_option_cookies); // TODO(jefftk): FlushEarlyFlow would go here. + bool page_callback_added = false; + ProxyFetchPropertyCallbackCollector* property_callback = + ProxyFetchFactory::InitiatePropertyCacheLookup( + !html_rewrite /* is_resource_fetch */, + url, + cfg_s->server_context, + options, + ctx->base_fetch, + false /* requires_blink_cohort (no longer unused) */, + &page_callback_added); // Will call StartParse etc. The rewrite driver will take care of deleting // itself if necessary. ctx->proxy_fetch = cfg_s->proxy_fetch_factory->CreateNewProxyFetch( url_string, ctx->base_fetch, driver, - property_callback.release(), + property_callback, NULL /* original_content_fetch */); return NGX_OK; } @@ -1865,6 +1862,9 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r, if (options->in_place_rewriting_enabled() && options->enabled() && options->IsAllowed(url.Spec())) { + ps_create_base_fetch(ctx, request_context, request_headers.release(), + kIproLookup); + // Do not store driver in request_context, it's not safe. RewriteDriver* driver; if (custom_options.get() == NULL) { @@ -1890,7 +1890,6 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r, url_string.c_str()); ctx->in_place = true; - ctx->base_fetch->set_ipro_lookup(true); ctx->driver->FetchInPlaceResource( url, false /* proxy_mode */, ctx->base_fetch); @@ -1903,9 +1902,7 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r, ngx_log_error(NGX_LOG_DEBUG, r->connection->log, 0, "Passing on content handling for non-pagespeed resource '%s'", url_string.c_str()); - - ctx->base_fetch->Done(false); - ps_release_base_fetch(ctx); + CHECK(ctx->base_fetch == NULL); // set html_rewrite flag. ctx->html_rewrite = true; return NGX_DECLINED; @@ -2690,11 +2687,9 @@ ngx_int_t ps_content_handler(ngx_http_request_t* r) { switch (response_category) { case RequestRouting::kError: return NGX_ERROR; - case RequestRouting::kNotUnderstood: case RequestRouting::kPagespeedDisabled: case RequestRouting::kInvalidUrl: case RequestRouting::kPagespeedSubrequest: - case RequestRouting::kNotHeadOrGet: case RequestRouting::kErrorResponse: return NGX_DECLINED; case RequestRouting::kBeacon: