release: 1.9.32.2 -> 1.9.32.3

NgxGZipSetter fix, at config reload Init did not work as expected (Issue 844)

config

@@ -27,8 +27,8 @@ if [ "$mod_pagespeed_dir" = "unset" ] ; then
     echo "   You need to separately download the pagespeed library:"
     echo ""
     echo "     $ cd /path/to/ngx_pagespeed"
-    echo "     $ wget https://dl.google.com/dl/page-speed/psol/1.9.32.1.tar.gz"
-    echo "     $ tar -xzvf 1.9.32.1.tar.gz # expands to psol/"
+    echo "     $ wget https://dl.google.com/dl/page-speed/psol/1.9.32.3.tar.gz"
+    echo "     $ tar -xzvf 1.9.32.3.tar.gz # expands to psol/"
     echo ""
     echo "   Or see the installation instructions:"
     echo "     https://github.com/pagespeed/ngx_pagespeed#how-to-build"
@@ -205,7 +205,7 @@ if [ $ngx_found = yes ]; then
 else
   cat << END
 $0: error: module ngx_pagespeed requires the pagespeed optimization library.
-Look in obj/autoconf.err for more details.
+Look in objs/autoconf.err for more details.
 END
   exit 1
 fi

src/ngx_base_fetch.cc

@@ -144,7 +144,7 @@ void NgxBaseFetch::HandleHeadersComplete() {
     }
   }
 
-  // For the IPRO lookup, supress notification of the nginx side here.
+  // For the IPRO lookup, suppress 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.

src/ngx_fetch.h

@@ -51,12 +51,13 @@ namespace net_instaweb {
 typedef bool (*response_handler_pt)(ngx_connection_t* c);
 
 class NgxUrlAsyncFetcher;
+class NgxConnection;
+
 class NgxFetch : public PoolElement<NgxFetch> {
  public:
   NgxFetch(const GoogleString& url,
            AsyncFetch* async_fetch,
            MessageHandler* message_handler,
-           ngx_msec_t timeout_ms,
            ngx_log_t* log);
   ~NgxFetch();
 
@@ -112,19 +113,19 @@ class NgxFetch : public PoolElement<NgxFetch> {
     response_handler = handler;
   }
   // Only the Static functions could be used in callbacks.
-  static void NgxFetchResolveDone(ngx_resolver_ctx_t* ctx);
+  static void ResolveDoneHandler(ngx_resolver_ctx_t* ctx);
   // Write the request.
-  static void NgxFetchWrite(ngx_event_t* wev);
+  static void ConnectionWriteHandler(ngx_event_t* wev);
   // Wait for the response.
-  static void NgxFetchRead(ngx_event_t* rev);
+  static void ConnectionReadHandler(ngx_event_t* rev);
   // Read and parse the first status line.
-  static bool NgxFetchHandleStatusLine(ngx_connection_t* c);
+  static bool HandleStatusLine(ngx_connection_t* c);
   // Read and parse the HTTP headers.
-  static bool NgxFetchHandleHeader(ngx_connection_t* c);
+  static bool HandleHeader(ngx_connection_t* c);
   // Read the response body.
-  static bool NgxFetchHandleBody(ngx_connection_t* c);
+  static bool HandleBody(ngx_connection_t* c);
   // Cancel the fetch when it's timeout.
-  static void NgxFetchTimeout(ngx_event_t* tev);
+  static void TimeoutHandler(ngx_event_t* tev);
 
   // Add the pagespeed User-Agent.
   void FixUserAgent();
@@ -139,7 +140,6 @@ class NgxFetch : public PoolElement<NgxFetch> {
   int64 bytes_received_;
   int64 fetch_start_ms_;
   int64 fetch_end_ms_;
-  int64 timeout_ms_;
   bool done_;
   int64 content_length_;
   bool content_length_known_;
@@ -152,7 +152,7 @@ class NgxFetch : public PoolElement<NgxFetch> {
   ngx_http_request_t* r_;
   ngx_http_status_t* status_;
   ngx_event_t* timeout_event_;
-  ngx_connection_t* connection_;
+  NgxConnection* connection_;
   ngx_resolver_ctx_t* resolver_ctx_;
 
   DISALLOW_COPY_AND_ASSIGN(NgxFetch);

src/ngx_gzip_setter.cc

@@ -70,7 +70,7 @@ extern "C" {
   }
 }
 
-NgxGZipSetter::NgxGZipSetter() : enabled_(0) { }
+NgxGZipSetter::NgxGZipSetter() : enabled_(false), initialized_(false) { }
 NgxGZipSetter::~NgxGZipSetter() { }
 
 // Helper functions to determine signature.
@@ -95,7 +95,7 @@ bool IsNgxBitmaskCommand(ngx_command_t* command) {
           HasLocalConfig(command));
 }
 
-// Initialize the NgxGzipSetter.
+// Initialize the NgxGZipSetter.
 // Find the gzip, gzip_vary, gzip_http_version and gzip_types commands in the
 // gzip module. Enable if the signature of the zip command matches with what we
 // trust. Also sets up redirects for the configurations. These redirect handle
@@ -105,6 +105,16 @@ void NgxGZipSetter::Init(ngx_conf_t* cf) {
 #if (NGX_HTTP_GZIP)
   bool gzip_signature_mismatch = false;
   bool other_signature_mismatch = false;
+  // If we initialized already we don't have to scan again.
+  if (initialized_) {
+    // Config might have changed, so re-enable if we have gzip.
+    if (gzip_command_.command_ != NULL) {
+      enabled_ = true;
+    } else {
+      enabled_ = false;
+    }
+    return;
+  } 
   for (int m = 0; ngx_modules[m] != NULL; m++) {
     if (ngx_modules[m]->commands != NULL) {
       for (int c = 0; ngx_modules[m]->commands[c].name.len; c++) {
@@ -122,7 +132,7 @@ void NgxGZipSetter::Init(ngx_conf_t* cf) {
             current_command->set = ngx_gzip_redirect_conf_set_flag_slot;
             gzip_command_.command_ = current_command;
             gzip_command_.module_ = ngx_modules[m];
-            enabled_ = 1;
+            enabled_ = true;
           } else {
             ngx_conf_log_error(
                 NGX_LOG_WARN, cf, 0,
@@ -189,6 +199,7 @@ void NgxGZipSetter::Init(ngx_conf_t* cf) {
       }
     }
   }
+  initialized_ = true;
   if (gzip_signature_mismatch) {
     return;  // Already logged error.
   } else if (!enabled_) {
@@ -381,7 +392,7 @@ void NgxGZipSetter::AddGZipHTTPTypes(ngx_conf_t* cf) {
 }
 
 void NgxGZipSetter::RollBackAndDisable(ngx_conf_t* cf) {
-  ngx_conf_log_error(NGX_LOG_INFO, cf, 0,
+  ngx_conf_log_error(NGX_LOG_DEBUG, cf, 0,
                      "pagespeed: rollback gzip, explicit configuration");
   for (std::vector<ngx_flag_t*>::iterator i = ngx_flags_set_.begin();
        i != ngx_flags_set_.end(); ++i) {

src/ngx_gzip_setter.h

@@ -91,6 +91,7 @@ class NgxGZipSetter {
   ngx_command_ctx gzip_vary_command_;
   ngx_command_ctx gzip_http_version_command_;
   bool enabled_;
+  bool initialized_;
 
  public:
   NgxGZipSetter();

src/ngx_pagespeed.cc

@@ -89,6 +89,8 @@ extern ngx_module_t ngx_pagespeed;
 namespace net_instaweb {
 
 const char* kInternalEtagName = "@psol-etag";
+bool factory_init_called = false;
+
 // The process context takes care of proactively initialising
 // a few libraries for us, some of which are not thread-safe
 // when they are initialized lazily.
@@ -605,6 +607,16 @@ char* ps_configure(ngx_conf_t* cf,
                    NgxRewriteOptions** options,
                    MessageHandler* handler,
                    net_instaweb::RewriteOptions::OptionScope option_scope) {
+  ps_main_conf_t* cfg_m = static_cast<ps_main_conf_t*>(
+      ngx_http_conf_get_module_main_conf(cf, ngx_pagespeed));
+
+  if (!factory_init_called) {
+    // Init logging to nginx's default error_log.
+    cfg_m->driver_factory->LoggingInit(cf->cycle->log);
+    cfg_m->driver_factory->Init();
+    factory_init_called = true;
+  }
+
   // args[0] is always "pagespeed"; ignore it.
   ngx_uint_t n_args = cf->args->nelts - 1;
 
@@ -655,8 +667,6 @@ char* ps_configure(ngx_conf_t* cf,
     // directive yet.  That happens below in ParseAndSetOptions().
   }
 
-  ps_main_conf_t* cfg_m = static_cast<ps_main_conf_t*>(
-      ngx_http_cycle_get_module_main_conf(cf->cycle, ngx_pagespeed));
   if (*options == NULL) {
     *options = new NgxRewriteOptions(
         cfg_m->driver_factory->thread_system());
@@ -725,7 +735,16 @@ void ps_cleanup_srv_conf(void* data) {
   // from being executed
 
   if (!factory_deleted && cfg_s->server_context != NULL) {
-    delete cfg_s->server_context->factory();
+    NgxRewriteDriverFactory* factory = dynamic_cast<NgxRewriteDriverFactory*>(
+      cfg_s->server_context->factory());
+
+    if (!factory_init_called) {
+      factory->LoggingInit(ngx_cycle->log);
+      factory->Init();
+      factory_init_called = true;
+    }
+
+    delete factory;
     factory_deleted = true;
   }
   if (cfg_s->proxy_fetch_factory != NULL) {
@@ -795,7 +814,7 @@ void* ps_create_main_conf(ngx_conf_t* cf) {
       new SystemThreadSystem(),
       "" /* hostname, not used */,
       -1 /* port, not used */);
-  cfg_m->driver_factory->Init();
+  factory_init_called = false;
   ps_set_conf_cleanup_handler(cf, ps_cleanup_main_conf, cfg_m);
   return cfg_m;
 }
@@ -1854,12 +1873,19 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r,
     CHECK(ctx == NULL);
     ctx = new ps_request_ctx_t();
 
+    ctx->base_fetch = NULL;
+    ctx->pagespeed_connection = NULL;
     ctx->r = r;
-    ctx->write_pending = false;
     ctx->html_rewrite = false;
     ctx->in_place = false;
-    ctx->pagespeed_connection = NULL;
+    ctx->write_pending = false;
+    ctx->fetch_done = false;
     ctx->preserve_caching_headers = kDontPreserveHeaders;
+    ctx->proxy_fetch = NULL;
+    ctx->inflater_ = NULL;
+    ctx->driver = NULL;
+    ctx->recorder = NULL;
+    ctx->ipro_response_headers = NULL;
 
     // See build_context_for_request() in mod_instaweb.cc
     // TODO(jefftk): Is this the right place to be modifying caching headers for
@@ -2673,8 +2699,8 @@ bool ps_request_body_to_string_piece(
 
     if (ret < 0) {
       ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
-		    "ps_request_body_to_string_piece: "
-		    "error reading post body.");
+                    "ps_request_body_to_string_piece: "
+                    "error reading post body.");
       return false;
     }
 
@@ -3084,10 +3110,15 @@ ngx_int_t ps_init_module(ngx_cycle_t* cycle) {
           "UseNativeFetcher is on, please configure a resolver.");
       return NGX_ERROR;
     }
-
+    // Update logging to the configured error_log in the http{} block.
     cfg_m->driver_factory->LoggingInit(cycle->log);
     cfg_m->driver_factory->RootInit();
   } else {
+    if (!factory_init_called) {
+      cfg_m->driver_factory->LoggingInit(cycle->log);
+      cfg_m->driver_factory->Init();
+      factory_init_called = true;
+    }
     delete cfg_m->driver_factory;
     cfg_m->driver_factory = NULL;
   }
@@ -3124,11 +3155,11 @@ ngx_int_t ps_init_child_process(ngx_cycle_t* cycle) {
     // Some server{} blocks may not have a ServerContext in that case we must
     // not instantiate a ProxyFetchFactory.
     if (cfg_s->server_context != NULL) {
-      cfg_s->proxy_fetch_factory = new ProxyFetchFactory(cfg_s->server_context);
       ngx_http_core_loc_conf_t* clcf = static_cast<ngx_http_core_loc_conf_t*>(
           cscfp[s]->ctx->loc_conf[ngx_http_core_module.ctx_index]);
       cfg_m->driver_factory->SetServerContextMessageHandler(
           cfg_s->server_context, clcf->error_log);
+      cfg_s->proxy_fetch_factory = new ProxyFetchFactory(cfg_s->server_context);
     }
   }
 

src/ngx_rewrite_driver_factory.cc

@@ -78,6 +78,8 @@ NgxRewriteDriverFactory::NgxRewriteDriverFactory(
       log_(NULL),
       resolver_timeout_(NGX_CONF_UNSET_MSEC),
       use_native_fetcher_(false),
+      // 100 Aligns to nginx's server-side default.
+      native_fetcher_max_keepalive_requests_(100),
       ngx_shared_circular_buffer_(NULL),
       hostname_(hostname.as_string()),
       port_(port),
@@ -112,6 +114,7 @@ UrlAsyncFetcher* NgxRewriteDriverFactory::AllocateFetcher(
         resolver_timeout_,
         config->blocking_fetch_timeout_ms(),
         resolver_,
+        native_fetcher_max_keepalive_requests_,
         thread_system(),
         message_handler());
     ngx_url_async_fetchers_.push_back(fetcher);

src/ngx_rewrite_driver_factory.h

@@ -105,6 +105,12 @@ class NgxRewriteDriverFactory : public SystemRewriteDriverFactory {
   void set_use_native_fetcher(bool x) {
     use_native_fetcher_ = x;
   }
+  int native_fetcher_max_keepalive_requests() {
+    return native_fetcher_max_keepalive_requests_;
+  }
+  void set_native_fetcher_max_keepalive_requests(int x) {
+    native_fetcher_max_keepalive_requests_ = x;
+  }
   bool process_script_variables() {
     return process_script_variables_;
   }
@@ -140,6 +146,8 @@ class NgxRewriteDriverFactory : public SystemRewriteDriverFactory {
   ngx_msec_t resolver_timeout_;
   ngx_resolver_t* resolver_;
   bool use_native_fetcher_;
+  int native_fetcher_max_keepalive_requests_;
+
   typedef std::set<NgxMessageHandler*> NgxMessageHandlerSet;
   NgxMessageHandlerSet server_context_message_handlers_;
 

src/ngx_rewrite_options.cc

@@ -72,12 +72,14 @@ const char* const server_only_options[] = {
   "LoadFromFileMatch",
   "LoadFromFileRule",
   "LoadFromFileRuleMatch",
-  "UseNativeFetcher"
+  "UseNativeFetcher",
+  "NativeFetcherMaxKeepaliveRequests"
 };
 
 // Options that can only be used in the main (http) option scope.
 const char* const main_only_options[] = {
-  "UseNativeFetcher"
+  "UseNativeFetcher",
+  "NativeFetcherMaxKeepaliveRequests"
 };
 
 }  // namespace
@@ -343,6 +345,16 @@ const char* NgxRewriteOptions::ParseAndSetOptions(
       result = ParseAndSetOptionHelper<NgxRewriteDriverFactory>(
           arg, driver_factory,
           &NgxRewriteDriverFactory::set_use_native_fetcher);
+    } else if (IsDirective(directive, "NativeFetcherMaxKeepaliveRequests")) {
+      int max_keepalive_requests;
+      if (StringToInt(arg, &max_keepalive_requests) &&
+          max_keepalive_requests > 0) {
+        driver_factory->set_native_fetcher_max_keepalive_requests(
+            max_keepalive_requests);
+        result = RewriteOptions::kOptionOk;
+      } else {
+        result = RewriteOptions::kOptionValueInvalid;
+      }
     } else if (StringCaseEqual("ProcessScriptVariables", args[0])) {
       if (scope == RewriteOptions::kProcessScopeStrict) {
         if (StringCaseEqual(arg, "on")) {

src/ngx_url_async_fetcher.cc

@@ -55,6 +55,7 @@ namespace net_instaweb {
                                          ngx_msec_t resolver_timeout,
                                          ngx_msec_t fetch_timeout,
                                          ngx_resolver_t* resolver,
+                                         int max_keepalive_requests,
                                          ThreadSystem* thread_system,
                                          MessageHandler* handler)
     : fetchers_count_(0),
@@ -63,7 +64,8 @@ namespace net_instaweb {
       byte_count_(0),
       thread_system_(thread_system),
       message_handler_(handler),
-      mutex_(NULL) {
+      mutex_(NULL),
+      max_keepalive_requests_(max_keepalive_requests) {
     resolver_timeout_ = resolver_timeout;
     fetch_timeout_ = fetch_timeout;
     ngx_memzero(&proxy_, sizeof(proxy_));
@@ -223,7 +225,7 @@ namespace net_instaweb {
                                  AsyncFetch* async_fetch) {
     async_fetch = EnableInflation(async_fetch);
     NgxFetch* fetch = new NgxFetch(url, async_fetch,
-          message_handler, fetch_timeout_, log_);
+          message_handler, log_);
     ScopedMutex lock(mutex_);
     pending_fetches_.Add(fetch);
     SendCmd('F');

src/ngx_url_async_fetcher.h

@@ -53,12 +53,13 @@ class NgxUrlAsyncFetcher : public UrlAsyncFetcher {
   NgxUrlAsyncFetcher(
       const char* proxy, ngx_log_t* log, ngx_msec_t resolver_timeout,
       ngx_msec_t fetch_timeout, ngx_resolver_t* resolver,
-      ThreadSystem* thread_system, MessageHandler* handler);
+      int max_keepalive_requests, ThreadSystem* thread_system,
+      MessageHandler* handler);
 
   ~NgxUrlAsyncFetcher();
 
   // It should be called in the module init_process callback function. Do some
-  // intializations which can't be done in the master process
+  // initializations which can't be done in the master process
   bool Init();
 
   // shutdown all the fetches.
@@ -139,6 +140,7 @@ class NgxUrlAsyncFetcher : public UrlAsyncFetcher {
   ngx_connection_t* command_connection_;  // the command pipe
   int pipe_fd_;  // the write pipe end
   ngx_resolver_t* resolver_;
+  int max_keepalive_requests_;
   ngx_msec_t resolver_timeout_;
   ngx_msec_t fetch_timeout_;
 

test/pagespeed_test.conf.template

@@ -38,6 +38,10 @@ http {
   pagespeed StaticAssetPrefix /pagespeed_custom_static/;
 
   pagespeed MessageBufferSize 200000;
+  # Increase the default fetcher timeout to resolve sporadic flakeyness when
+  # the native fetcher uses 8.8.8.8 to resolve.
+  pagespeed FetcherTimeoutMs 10000;
+  pagespeed NativeFetcherMaxKeepaliveRequests 50;
 
   root "@@SERVER_ROOT@@";