Merge pull request #884 from pagespeed/oschaaf-trunk-tracking-base-fetch

base-fetch: prevent unused constructions and prop. cache lookups
2015-01-23 20:09:45 +01:00
parent 7d965aced7 a5411a1c7c
commit d0858790e4
3 changed files with 78 additions and 52 deletions
@@ -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
@@ -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;
  }
 }

@@ -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);
 };
@@ -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: