artox/incubator-pagespeed-ngx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

base-fetch: prevent unused constructions and prop. cache lookups

Browse Source 
- Don't construct `NgxBaseFetch` instances and call `Done(false)` later when it turns out it wasn't needed.
- Track the purpose of the base fetch instead of just wether it is an IPRO lookup or not for more informational tracing
  as well as avoiding constructing base fetches when their purpose isn't know yet.
- Don't initiate a property cache lookup when we won't need it (`ProxyFetchFactory::InitiatePropertyCacheLookup`)
- Improve IPRO handling of `NgxBaseFetch` and make it clearer. Indicate we want to supress further events when the
  base fetch was used to lookup an IPRO url, and the result was not indicative of something we can respond with to the
  browser.
- Remove unused `RequestRouting` constants
This commit is contained in:
Otto van der Schaaf
2015-01-19 13:26:44 +01:00
parent f2e7b15d9c
commit a5411a1c7c
3 changed files with 78 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ngx_base_fetch.cc
+37 -12
View File
@@ -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<NgxBaseFetch*>(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
@@ -99,7 +117,8 @@ void NgxBaseFetch::ReadCallback(const ps_event_data& data) {
// 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);
ngx_http_finalize_request(r, NGX_ERROR);
@@ -184,6 +203,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
@@ -199,19 +222,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;
}
}
src/ngx_base_fetch.h
+10 -4
View File
@@ -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);
};
src/ngx_pagespeed.cc
+31 -36
View File
@@ -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<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));
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: