1. bugfix, connection_pool_mutex may not got unlock

2. disable keepalive if we don't find keep-alive header

README.md

@@ -37,11 +37,11 @@ recompiling Tengine](https://github.com/pagespeed/ngx_pagespeed/wiki/Using-ngx_p
 
    ```bash
    $ cd ~
-   $ wget https://github.com/pagespeed/ngx_pagespeed/archive/v1.7.30.3-beta.zip
-   $ unzip v1.7.30.3-beta.zip # or unzip v1.7.30.3-beta
-   $ cd ngx_pagespeed-1.7.30.3-beta/
-   $ wget https://dl.google.com/dl/page-speed/psol/1.7.30.3.tar.gz
-   $ tar -xzvf 1.7.30.3.tar.gz # expands to psol/
+   $ wget https://github.com/pagespeed/ngx_pagespeed/archive/v1.7.30.4-beta.zip
+   $ unzip v1.7.30.4-beta.zip # or unzip v1.7.30.4-beta
+   $ cd ngx_pagespeed-1.7.30.4-beta/
+   $ wget https://dl.google.com/dl/page-speed/psol/1.7.30.4.tar.gz
+   $ tar -xzvf 1.7.30.4.tar.gz # expands to psol/
    ```
 
 3. Download and build nginx:
@@ -49,10 +49,10 @@ recompiling Tengine](https://github.com/pagespeed/ngx_pagespeed/wiki/Using-ngx_p
    ```bash
    $ cd ~
    $ # check http://nginx.org/en/download.html for the latest version
-   $ wget http://nginx.org/download/nginx-1.4.4.tar.gz
-   $ tar -xvzf nginx-1.4.4.tar.gz
-   $ cd nginx-1.4.4/
-   $ ./configure --add-module=$HOME/ngx_pagespeed-1.7.30.3-beta
+   $ wget http://nginx.org/download/nginx-1.6.0.tar.gz
+   $ tar -xvzf nginx-1.6.0.tar.gz
+   $ cd nginx-1.6.0/
+   $ ./configure --add-module=$HOME/ngx_pagespeed-1.7.30.4-beta
    $ make
    $ sudo make install
    ```
@@ -95,7 +95,7 @@ To confirm that the module is loaded, fetch a page and check that you see the
 
 ```bash
 $ curl -I 'http://localhost:8050/some_page/' | grep X-Page-Speed
-X-Page-Speed: 1.7.30.3-...
+X-Page-Speed: 1.7.30.4-...
 ```
 
 Looking at the source of a few pages you should see various changes, such as

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.7.30.2.tar.gz"
-    echo "     $ tar -xzvf 1.7.30.2.tar.gz # expands to psol/"
+    echo "     $ wget https://dl.google.com/dl/page-speed/psol/1.7.30.4.tar.gz"
+    echo "     $ tar -xzvf 1.7.30.4.tar.gz # expands to psol/"
     echo ""
     echo "   Or see the installation instructions:"
     echo "     https://github.com/pagespeed/ngx_pagespeed#how-to-build"
@@ -111,6 +111,10 @@ case "$NGX_GCC_VER" in
   ;;
 esac
 
+if [ "$WNO_ERROR" = "YES" ]; then
+    CFLAGS="$CFLAGS -Wno-error"
+fi
+
 pagespeed_include="\
   $mod_pagespeed_dir \
   $mod_pagespeed_dir/third_party/chromium/src \

src/ngx_fetch.cc

@@ -54,8 +54,156 @@ extern "C" {
 #include "net/instaweb/util/public/thread_system.h"
 #include "net/instaweb/util/public/timer.h"
 #include "net/instaweb/util/public/writer.h"
+#include "net/instaweb/util/public/pthread_mutex.h"
 
 namespace net_instaweb {
+  class NgxConnection : public PoolElement<NgxConnection> {
+  public:
+    NgxConnection();
+    ~NgxConnection();
+    void SetKeepAlive(bool k = true) { keepalive_ = k; }
+    bool KeepAlive() { return keepalive_; }
+    void SetSock(u_char *sockaddr, socklen_t socklen) {
+      socklen_ = socklen;
+      ngx_memcpy(&sockaddr_, sockaddr, socklen);
+    }
+
+    static NgxConnection* Connect(ngx_peer_connection_t *pc);
+    void Close();
+    static void NgxConnectionDumyHandler(ngx_event_t *ev) {};
+    static void NgxConnectionCloseHandler(ngx_event_t *ev);
+
+    typedef Pool<NgxConnection> NgxConnectionPool;
+
+    static NgxConnectionPool connection_pool;
+    static PthreadMutex connection_pool_mutex;
+    ngx_connection_t *c_;
+  
+  private:
+    int64 timeout_;
+    int max_requests_;
+    bool keepalive_;
+    socklen_t socklen_;
+    u_char sockaddr_[NGX_SOCKADDRLEN];
+  };
+
+  NgxConnection::NgxConnectionPool NgxConnection::connection_pool;
+  PthreadMutex NgxConnection::connection_pool_mutex;
+
+  NgxConnection::NgxConnection() {
+    c_ = NULL;
+    keepalive_ = false;
+
+    // default keepalive 60s, max process 100 requests
+    timeout_ = 60000;
+    max_requests_ = 100;
+  }
+
+  NgxConnection::~NgxConnection() {
+    //
+  }
+
+  NgxConnection* NgxConnection::Connect(ngx_peer_connection_t *pc) {
+    NgxConnection *nc;
+
+    NgxConnection::connection_pool_mutex.Lock();
+    for (Pool<NgxConnection>::iterator p = connection_pool.begin();
+         p != connection_pool.end(); p++) {
+      nc = *p;
+      if (ngx_memn2cmp(static_cast<u_char*>(nc->sockaddr_),
+                       reinterpret_cast<u_char*>(pc->sockaddr),
+                       nc->socklen_, pc->socklen) == 0) {
+        nc->c_->idle = 0;
+        nc->c_->log = pc->log;
+        nc->c_->read->log = pc->log;
+        nc->c_->write->log = pc->log;
+        nc->c_->pool->log = pc->log;
+
+        if (nc->c_->read->timer_set) {
+          ngx_del_timer(nc->c_->read);
+        }
+
+        NgxConnection::connection_pool_mutex.Unlock();
+        return nc;
+      }
+    }
+    connection_pool_mutex.Unlock();
+
+    int rc = ngx_event_connect_peer(pc);
+    if (rc == NGX_ERROR || rc == NGX_DECLINED || rc == NGX_BUSY) {
+      return NULL;
+    }
+
+    nc = new NgxConnection();
+    nc->SetSock(reinterpret_cast<u_char*>(pc->sockaddr), pc->socklen);
+    nc->c_ = pc->connection;
+
+    return nc;
+  }
+
+  void NgxConnection::Close() {
+    max_requests_--;
+    if (!keepalive_ || max_requests_ <= 0) {
+      ngx_close_connection(c_);
+      delete this;
+      return;
+    }
+
+    if (c_->read->timer_set) {
+      ngx_del_timer(c_->read);
+    }
+
+    if (c_->write->timer_set) {
+      ngx_del_timer(c_->write);
+    }
+
+    ngx_add_timer(c_->read, static_cast<ngx_msec_t>(timeout_));
+
+    c_->data = this;
+    c_->read->handler = NgxConnectionCloseHandler;
+    c_->write->handler = NgxConnectionDumyHandler;
+    c_->idle = 1;
+
+    // this connection should not be associated with current fetch
+    c_->log = ngx_cycle->log;
+    c_->read->log = ngx_cycle->log;
+    c_->write->log = ngx_cycle->log;
+    c_->pool->log = ngx_cycle->log;
+
+    connection_pool_mutex.Lock();
+    connection_pool.Add(this);
+    connection_pool_mutex.Unlock();
+  }
+
+  void NgxConnection::NgxConnectionCloseHandler(ngx_event_t *ev) {
+    ngx_connection_t *c = static_cast<ngx_connection_t*>(ev->data);
+    NgxConnection *nc = static_cast<NgxConnection*>(c->data);
+
+    if (c->read->timedout) {
+      nc->SetKeepAlive(false);
+      nc->Close();
+      return;
+    }
+
+    char buf[1];
+    int n;
+
+    // not a timedout event, we should check connection
+    n = recv(c->fd, buf, 1, MSG_PEEK);
+    if (n == -1 && ngx_socket_errno == NGX_EAGAIN) {
+      if (ngx_handle_read_event(c->read, 0) != NGX_OK) {
+        nc->SetKeepAlive(false);
+        nc->Close();
+        return;
+      }
+
+      return;
+    }
+
+    nc->SetKeepAlive(false);
+    nc->Close();
+  }
+
   NgxFetch::NgxFetch(const GoogleString& url,
                      AsyncFetch* async_fetch,
                      MessageHandler* message_handler,
@@ -70,7 +218,7 @@ namespace net_instaweb {
         fetch_start_ms_(0),
         fetch_end_ms_(0),
         done_(false),
-        content_length_(0) {
+        content_length_(-1) {
             ngx_memzero(&url_, sizeof(url_));
             log_ = log;
             pool_ = NULL;
@@ -83,7 +231,8 @@ namespace net_instaweb {
         ngx_del_timer(timeout_event_);
     }
     if (connection_ != NULL) {
-      ngx_close_connection(connection_);
+      connection_->Close();
+      connection_ = NULL;
     }
     if (pool_ != NULL) {
       ngx_destroy_pool(pool_);
@@ -216,8 +365,31 @@ namespace net_instaweb {
       ngx_del_timer(timeout_event_);
       timeout_event_ = NULL;
     }
+
+    if (success) {
+      ConstStringStarVector v;
+      if (async_fetch_->response_headers()->Lookup(
+            StringPiece(HttpAttributes::kConnection), &v)) {
+        bool keepalive = false;
+        for (int i = 0; i < v.size(); i++) {
+          if (*v[i] == "keep-alive") {
+            keepalive = true;
+            break;
+
+          } else if (*v[i] == "close") {
+            break;
+          }
+        }
+
+        // - enable keepalive if we find "keep-alive" header
+        // - disable keepalive, if it's with "Connection:close"
+        // - disable keepalive, if it's without "keep-alive" header
+        connection_->SetKeepAlive(keepalive);
+      }
+    }
+
     if (connection_) {
-      ngx_close_connection(connection_);
+      connection_->Close();
       connection_ = NULL;
     }
 
@@ -408,7 +580,7 @@ namespace net_instaweb {
       return rc;
     }
 
-    NgxFetchWrite(connection_->write);
+    NgxFetchWrite(connection_->c_->write);
     return NGX_OK;
   }
 
@@ -425,18 +597,18 @@ namespace net_instaweb {
     pc.log = fetcher_->log_;
     pc.rcvbuf = -1;
 
-    int rc = ngx_event_connect_peer(&pc);
-    if (rc == NGX_ERROR || rc == NGX_DECLINED || rc == NGX_BUSY) {
-      return rc;
+
+    connection_ = NgxConnection::Connect(&pc);
+    if (connection_ == NULL) {
+      return NGX_ERROR;
     }
 
-    connection_ = pc.connection;
-    connection_->write->handler = NgxFetchWrite;
-    connection_->read->handler = NgxFetchRead;
-    connection_->data = this;
+    connection_->c_->write->handler = NgxFetchWrite;
+    connection_->c_->read->handler = NgxFetchRead;
+    connection_->c_->data = this;
 
     // Timer set in Init() is still in effect.
-    return rc;
+    return NGX_OK;
   }
 
   // When the fetch sends the request completely, it will hook the read event,

src/ngx_fetch.h

@@ -42,6 +42,7 @@ 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,
@@ -136,7 +137,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_pagespeed.cc

@@ -246,11 +246,11 @@ void copy_response_headers_from_ngx(const ngx_http_request_t* r,
 
   // When we don't have a date header, invent one.
   const char* date = headers->Lookup1(HttpAttributes::kDate);
-  
+
   if (date == NULL) {
     headers->SetDate(ngx_current_msec);
   }
-  
+
   // TODO(oschaaf): ComputeCaching should be called in setupforhtml()?
   headers->ComputeCaching();
 }
@@ -450,7 +450,7 @@ ngx_command_t ps_commands[] = {
     NULL },
 
   { ngx_string("pagespeed"),
-    NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1|
+    NGX_HTTP_LOC_CONF|NGX_HTTP_LIF_CONF|NGX_CONF_TAKE1|
     NGX_CONF_TAKE2|NGX_CONF_TAKE3|NGX_CONF_TAKE4|NGX_CONF_TAKE5,
     ps_loc_configure,
     NGX_HTTP_SRV_CONF_OFFSET,
@@ -570,15 +570,24 @@ char* ps_init_dir(const StringPiece& directive,
     return NULL;  // We're not root, so we're staying whoever we are.
   }
 
+  // chown if owner differs from nginx worker user.
   ngx_core_conf_t* ccf =
       (ngx_core_conf_t*)(ngx_get_conf(cf->cycle->conf_ctx, ngx_core_module));
   CHECK(ccf != NULL);
-
-  if (chown(gs_path.c_str(), ccf->user, ccf->group) != 0) {
+  struct stat gs_stat;
+  if (stat(gs_path.c_str(), &gs_stat) != 0) {
     return string_piece_to_pool_string(
         cf->pool, net_instaweb::StrCat(
-            directive, " ", path, " unable to set permissions"));
+            directive, " ", path, " stat() failed"));
   }
+  if (gs_stat.st_uid != ccf->user) {
+    if (chown(gs_path.c_str(), ccf->user, ccf->group) != 0) {
+      return string_piece_to_pool_string(
+          cf->pool, net_instaweb::StrCat(
+              directive, " ", path, " unable to set permissions"));
+    }
+  }
+
   return NULL;
 }
 
@@ -855,19 +864,28 @@ char* ps_merge_srv_conf(ngx_conf_t* cf, void* parent, void* child) {
 }
 
 char* ps_merge_loc_conf(ngx_conf_t* cf, void* parent, void* child) {
-  ps_loc_conf_t* parent_cfg_l = static_cast<ps_loc_conf_t*>(parent);
-
-  // The variant of the pagespeed directive that is acceptable in location
-  // blocks is only acceptable in location blocks, so we should never be merging
-  // in options from a server or main block.
-  CHECK(parent_cfg_l->options == NULL);
-
   ps_loc_conf_t* cfg_l = static_cast<ps_loc_conf_t*>(child);
   if (cfg_l->options == NULL) {
     // No directory specific options.
     return NGX_CONF_OK;
   }
 
+  // While you can't put a "location" block inside a "location" block you can
+  // put an "if" block inside a "location" block, which is implemented by making
+  // a pretend "location" block.  In this case we may have pagespeed options
+  // from the parent "location" block as well as from the current locationish
+  // "if" block.
+  ps_loc_conf_t* parent_cfg_l = static_cast<ps_loc_conf_t*>(parent);
+  if (parent_cfg_l->options != NULL) {
+    // Rebase our options off of the ones defined in the parent location block.
+    ps_merge_options(parent_cfg_l->options, &cfg_l->options);
+    return NGX_CONF_OK;
+  }
+
+  // Pagespeed options are defined in this location block, and it either has no
+  // parent (typical case) or is an if block whose parent location block defines
+  // no pagespeed options.  Base our options off of those in the server block.
+
   ps_srv_conf_t* cfg_s = static_cast<ps_srv_conf_t*>(
       ngx_http_conf_get_module_srv_conf(cf, ngx_pagespeed));
 
@@ -2184,6 +2202,7 @@ ngx_int_t ps_in_place_check_header_filter(ngx_http_request_t* r) {
     // (or at least a note that it cannot be cached stored there).
     // We do that using an Apache output filter.
     ctx->recorder = new InPlaceResourceRecorder(
+        RequestContextPtr(cfg_s->server_context->NewRequestContext(r)),
         url,
         request_headers.release(),
         options->respect_vary(),
@@ -2247,7 +2266,9 @@ ngx_int_t ps_in_place_body_filter(ngx_http_request_t* r, ngx_chain_t* in) {
     // Unlike in Apache we get the final response headers before we get the
     // content.  This means we can consider them earlier and abort the
     // request if need be without buffering everything.
-    recorder->ConsiderResponseHeaders(ctx->ipro_response_headers);
+    recorder->ConsiderResponseHeaders(
+        InPlaceResourceRecorder::kPreliminaryHeaders,
+        ctx->ipro_response_headers);
   }
 
   for (ngx_chain_t* cl = in; cl; cl = cl->next) {

test/nginx_system_test.sh

@@ -271,6 +271,28 @@ function run_post_cache_flush() {
 
 # nginx-specific system tests
 
+start_test Test pagespeed directive inside if block inside location block.
+
+URL="http://if-in-location.example.com/"
+URL+="mod_pagespeed_example/inline_javascript.html"
+
+# When we specify the X-Custom-Header-Inline-Js that triggers an if block in the
+# config which turns on inline_javascript.
+WGET_ARGS="--header=X-Custom-Header-Inline-Js:Yes"
+http_proxy=$SECONDARY_HOSTNAME \
+  fetch_until $URL 'grep -c document.write' 1
+OUT=$(http_proxy=$SECONDARY_HOSTNAME $WGET_DUMP $WGET_ARGS $URL)
+check_from "$OUT" fgrep "X-Inline-Javascript: Yes"
+check_not_from "$OUT" fgrep "inline_javascript.js"
+
+# Without that custom header we don't trigger the if block, and shouldn't get
+# any inline javascript.
+WGET_ARGS=""
+OUT=$(http_proxy=$SECONDARY_HOSTNAME $WGET_DUMP $WGET_ARGS $URL)
+check_from "$OUT" fgrep "X-Inline-Javascript: No"
+check_from "$OUT" fgrep "inline_javascript.js"
+check_not_from "$OUT" fgrep "document.write"
+
 # Tests related to rewritten response (downstream) caching.
 
 if [ "$NATIVE_FETCHER" = "on" ]; then

test/pagespeed_test.conf.template

@@ -127,6 +127,45 @@ http {
     }
   }
 
+  server {
+    listen @@SECONDARY_PORT@@;
+    server_name if-in-server.example.com;
+    pagespeed FileCachePath "@@SECONDARY_CACHE@@";
+
+    pagespeed RewriteLevel PassThrough;
+    set $inline_javascript "No";
+
+    if ($http_x_custom_header_inline_js) {
+      # TODO(jefftk): Turn on NGX_HTTP_SIF_CONF and figure out how to get
+      # pagespeed directives inside of a server location block to be respected,
+      # then uncomment the following line and duplicate the if-in-location test
+      # for if-in-server.
+      #pagespeed EnableFilters inline_javascript;
+      set $inline_javascript "Yes";
+    }
+
+    add_header "X-Inline-Javascript" $inline_javascript;
+  }
+
+  server {
+    listen @@SECONDARY_PORT@@;
+    server_name if-in-location.example.com;
+    pagespeed FileCachePath "@@SECONDARY_CACHE@@";
+
+
+    location / {
+      set $inline_javascript "No";
+      pagespeed RewriteLevel PassThrough;
+
+      if ($http_x_custom_header_inline_js) {
+        pagespeed EnableFilters inline_javascript;
+        set $inline_javascript "Yes";
+      }
+
+      add_header "X-Inline-Javascript" $inline_javascript;
+    }
+  }
+
   server {
     listen @@SECONDARY_PORT@@;
     server_name mpd.example.com;