Set buildtype from env PSOL_BUILDTYPE. (#1486)

Bump the pipe capacity, because running out of buffer space may cause
a write to spin indefinitely on EAGAIN.
Bumping the pipe capacity should eliminate the problem in practice,
though in theory the module could still be subject to it.
For now, leaving behind a todo with a suggested solution (should
the problem ever show up again).

Fixes https://github.com/pagespeed/ngx_pagespeed/issues/1380

config

@@ -15,9 +15,11 @@
 # Environment Variables (Optional):
 #   MOD_PAGESPEED_DIR: absolute path to the mod_pagespeed/src directory
 #   PSOL_BINARY: absolute path to pagespeed_automatic.a
+#   PSOL_BUILDTYPE: Release or Debug
 
 mod_pagespeed_dir="${MOD_PAGESPEED_DIR:-unset}"
 position_aux="${POSITION_AUX:-unset}"
+psol_buildtype="${PSOL_BUILDTYPE:-unset}"
 
 if [ "$mod_pagespeed_dir" = "unset" ] ; then
   mod_pagespeed_dir="$ngx_addon_dir/psol/include"
@@ -75,12 +77,16 @@ else
   exit 1
 fi
 
-if [ "$NGX_DEBUG" = "YES" ]; then
-  buildtype=Debug
-  # If we're using a psol tarball that doesn't contain Debug/ (which is the case
-  # from 1.12 onward) then this will be overriden to buildtype=Release below.
+if [ "$psol_buildtype" = "unset" ] ; then
+  if [ "$NGX_DEBUG" = "YES" ]; then
+    buildtype=Debug
+    # If we're using a psol tarball that doesn't contain Debug/ (which is the case
+    # from 1.12 onward) then this will be overriden to buildtype=Release below.
+  else
+    buildtype=Release
+  fi
 else
-  buildtype=Release
+  buildtype=$psol_buildtype
 fi
 
 # If the compiler is gcc, we want to use g++ to link, if at all possible,

scripts/build_ngx_pagespeed.sh

@@ -634,7 +634,13 @@ Not deleting $directory; name is suspiciously short.  Something is wrong."
       run make apache_debug_psol
       PSOL_BINARY="$MOD_PAGESPEED_DIR/out/$BUILD_TYPE/pagespeed_automatic.a"
     else
-      run install/build_psol.sh --skip_tests --skip_packaging
+      if "$DO_DEPS_CHECK"; then
+        skip_deps_arg=""
+      else
+        skip_deps_arg="--skip_deps"
+      fi
+
+      run install/build_psol.sh --skip_tests --skip_packaging "$skip_deps_arg"
       PSOL_BINARY="$MOD_PAGESPEED_DIR/pagespeed/automatic/pagespeed_automatic.a"
     fi
     run popd

src/ngx_event_connection.cc

@@ -52,6 +52,12 @@ bool NgxEventConnection::Init(ngx_cycle_t* cycle) {
   } else {
     pipe_read_fd_ = file_descriptors[0];
     pipe_write_fd_ = file_descriptors[1];
+    // Attempt to bump the pipe capacity, because running out of buffer space
+    // can potentially lead up to writes spinning on EAGAIN.
+    // See https://github.com/pagespeed/ngx_pagespeed/issues/1380
+    // TODO(oschaaf): Consider implementing a queueing mechanism for retrying
+    // failed writes.
+    fcntl(pipe_write_fd_, F_SETPIPE_SZ, 200*1024 /* minimal amount of bytes */);
     return true;
   }
   close(file_descriptors[0]);

src/ngx_fetch.cc

@@ -140,8 +140,6 @@ NgxConnection* NgxConnection::Connect(ngx_peer_connection_t* pc,
 
   int rc = ngx_event_connect_peer(pc);
   if (rc == NGX_ERROR || rc == NGX_DECLINED || rc == NGX_BUSY) {
-    ngx_log_error(NGX_LOG_DEBUG, pc->log, 0,
-                    "NgxFetch: ngx_event_connect_peer failed");
     return NULL;
   }
 
@@ -365,17 +363,14 @@ bool NgxFetch::Init() {
 
   GoogleString s_ipaddress(reinterpret_cast<char*>(tmp_url->host.data),
                            tmp_url->host.len);
-
   ngx_memzero(&sin_, sizeof(sin_));
-  if (s_ipaddress[0] == '[') {
-      s_ipaddress = s_ipaddress.substr(1);
-  }
-  if (s_ipaddress[s_ipaddress.size() - 1] == ']') {
-      s_ipaddress = s_ipaddress.substr(0, s_ipaddress.size()-2);      
-  }
-  ngx_int_t res = ngx_parse_addr(pool_, &sin_, (u_char*)s_ipaddress.c_str(), s_ipaddress.size());
-  if (res != NGX_OK) {
-    // Hostname isn't a valid IP address.  Check DNS.
+  sin_.sin_family = AF_INET;
+  sin_.sin_port = htons(tmp_url->port);
+  sin_.sin_addr.s_addr = inet_addr(s_ipaddress.c_str());
+
+  if (sin_.sin_addr.s_addr == INADDR_NONE) {
+    // inet_addr returned INADDR_NONE, which means the hostname
+    // isn't a valid IP address.  Check DNS.
     ngx_resolver_ctx_t temp;
     temp.name.data = tmp_url->host.data;
     temp.name.len = tmp_url->host.len;
@@ -410,16 +405,6 @@ bool NgxFetch::Init() {
       return false;
     }
   } else {
-      /*
-    u_char text[NGX_SOCKADDR_STRLEN];
-    ngx_str_t addrtext;
-    addrtext.len = ngx_sock_ntop(fetch->sin_.sockaddr, fetch->sin_.socklen, 
-        text, NGX_SOCKADDR_STRLEN, htons(fetcher->proxy_.url.len ? htons(fetcher->proxy_.port) : fetch->url_.port));
-    ngx_log_error(NGX_LOG_DEBUG, fetch->log_, 0,
-                    "NgxFetch %p: Resolved host [%V] to [%V]", fetch,
-                    &resolver_ctx->name, &addrtext);
-    */
-    ((struct sockaddr_in *)sin_.sockaddr)->sin_port = htons(fetcher_->proxy_.url.len ? htons(fetcher_->proxy_.port) : url_.port);
     if (InitRequest() != NGX_OK) {
       message_handler()->Message(kError, "NgxFetch: InitRequest failed");
       return false;
@@ -550,14 +535,14 @@ void NgxFetch::ResolveDoneHandler(ngx_resolver_ctx_t* resolver_ctx) {
   }
 
   ngx_uint_t i;
-  // Find the first ipv4 address. We don't support ipv6 yet in dns results.
+  // Find the first ipv4 address. We don't support ipv6 yet.
   for (i = 0; i < resolver_ctx->naddrs; i++) {
     // Old versions of nginx and tengine have a different definition of addrs,
     // work around to make sure we are using the right type (ngx_addr_t*).
     ngx_addr_t* ngx_addrs = reinterpret_cast<ngx_addr_t*>(resolver_ctx->addrs);
     if (typeid(*ngx_addrs) == typeid(*resolver_ctx->addrs)) {
       if (reinterpret_cast<struct sockaddr_in*>(ngx_addrs[i].sockaddr)
-             ->sin_family == AF_INET) {
+              ->sin_family == AF_INET) {
         break;
       }
     } else {
@@ -581,30 +566,37 @@ void NgxFetch::ResolveDoneHandler(ngx_resolver_ctx_t* resolver_ctx) {
   ngx_memzero(&fetch->sin_, sizeof(fetch->sin_));
 
 #if (nginx_version < 1005008)
-  // TODO(oschaaf): need to test this again, might be broken.
   fetch->sin_.sin_addr.s_addr = resolver_ctx->addrs[i];
 #else
-  fetch->sin_ = resolver_ctx->addrs[i];
+  struct sockaddr_in* sin;
+
+  sin = reinterpret_cast<struct sockaddr_in*>(
+      resolver_ctx->addrs[i].sockaddr);
+
+  fetch->sin_.sin_family = sin->sin_family;
+  fetch->sin_.sin_addr.s_addr = sin->sin_addr.s_addr;
 #endif
 
-  // Maybe we have Proxy
-  ((struct sockaddr_in *)fetch->sin_.sockaddr)->sin_port = 
-    htons(fetcher->proxy_.url.len ? htons(fetcher->proxy_.port) : fetch->url_.port);
+  fetch->sin_.sin_family = AF_INET;
+  fetch->sin_.sin_port = htons(fetch->url_.port);
+
+  // Maybe we have Proxy
+  if (0 != fetcher->proxy_.url.len) {
+    fetch->sin_.sin_port = htons(fetcher->proxy_.port);
+  }
+
+  char* ip_address = inet_ntoa(fetch->sin_.sin_addr);
 
-  u_char text[NGX_SOCKADDR_STRLEN];
-  ngx_str_t addrtext;
-  addrtext.len = ngx_sock_ntop(fetch->sin_.sockaddr, fetch->sin_.socklen, 
-    text, NGX_SOCKADDR_STRLEN, htons(fetcher->proxy_.url.len ? htons(fetcher->proxy_.port) : fetch->url_.port));
-  addrtext.data = text;
   ngx_log_error(NGX_LOG_DEBUG, fetch->log_, 0,
-                "NgxFetch %p: Resolved host [%V] to [%V]", fetch,
-                &resolver_ctx->name, &addrtext);
+                "NgxFetch %p: Resolved host [%V] to [%s]", fetch,
+                &resolver_ctx->name, ip_address);
+
+  fetch->release_resolver();
 
   if (fetch->InitRequest() != NGX_OK) {
     fetch->message_handler()->Message(kError, "NgxFetch: InitRequest failed");
     fetch->CallbackDone(false);
   }
-  fetch->release_resolver();
 }
 
 // Prepare the request data for this fetch, and hook the write event.
@@ -700,8 +692,8 @@ int NgxFetch::InitRequest() {
 int NgxFetch::Connect() {
   ngx_peer_connection_t pc;
   ngx_memzero(&pc, sizeof(pc));
-  pc.sockaddr =  sin_.sockaddr;
-  pc.socklen = sin_.socklen;
+  pc.sockaddr = (struct sockaddr*)&sin_;
+  pc.socklen = sizeof(struct sockaddr_in);
   pc.name = &url_.host;
 
   // get callback is dummy function, it just returns NGX_OK

src/ngx_fetch.h

@@ -191,8 +191,7 @@ class NgxFetch : public PoolElement<NgxFetch> {
   int64 content_length_;
   bool content_length_known_;
 
-
-  ngx_addr_t sin_;
+  struct sockaddr_in sin_;
   ngx_log_t* log_;
   ngx_buf_t* out_;
   ngx_buf_t* in_;

src/ngx_pagespeed.cc

@@ -84,6 +84,7 @@ extern ngx_module_t ngx_pagespeed;
 
 // Needed for SystemRewriteDriverFactory to use shared memory.
 #define PAGESPEED_SUPPORT_POSIX_SHARED_MEM
+#define NGINX_1_13_4 1013004
 
 net_instaweb::NgxRewriteDriverFactory* active_driver_factory = NULL;
 
@@ -2703,8 +2704,7 @@ ngx_int_t ps_simple_handler(ngx_http_request_t* r,
     }
   }
 
-  send_out_headers_and_body(r, response_headers, output);
-  return NGX_OK;
+  return send_out_headers_and_body(r, response_headers, output);
 }
 
 void ps_beacon_handler_helper(ngx_http_request_t* r,
@@ -3019,13 +3019,17 @@ ngx_int_t ps_preaccess_handler(ngx_http_request_t* r) {
   ph = cmcf->phase_engine.handlers;
 
   i = r->phase_handler;
+
   // move handlers before try_files && content phase
+  // As of nginx 1.13.4 we will be right before the try_files module
+  #if (nginx_version < NGINX_1_13_4)
   while (ph[i + 1].checker != ngx_http_core_try_files_phase &&
          ph[i + 1].checker != ngx_http_core_content_phase) {
     ph[i] = ph[i + 1];
     ph[i].next--;
     i++;
   }
+  #endif
 
   // insert ps phase handler
   ph[i].checker = ps_phase_handler;
@@ -3081,8 +3085,17 @@ ngx_int_t ps_init(ngx_conf_t* cf) {
     ngx_http_core_main_conf_t* cmcf = static_cast<ngx_http_core_main_conf_t*>(
         ngx_http_conf_get_module_main_conf(cf, ngx_http_core_module));
 
+    int phase = NGX_HTTP_PREACCESS_PHASE;
+
+    // As of nginx 1.13.4, try_files has changed.
+    // https://github.com/nginx/nginx/commit/129b06dc5dfab7b4513a4f274b3778cd9b8a6a22
+#if (nginx_version >= NGINX_1_13_4)
+    phase = NGX_HTTP_PRECONTENT_PHASE;
+#endif
+
     ngx_http_handler_pt* h = static_cast<ngx_http_handler_pt*>(
-        ngx_array_push(&cmcf->phases[NGX_HTTP_PREACCESS_PHASE].handlers));
+        ngx_array_push(&cmcf->phases[phase].handlers));
+
     if (h == NULL) {
       return NGX_ERROR;
     }

test/pagespeed_test.conf.template

@@ -88,6 +88,16 @@ http {
   # Expanded to CentralControllerPort directive when RUN_CONTROLLER_TEST=on
   @@CONTROLLER@@
 
+  server {
+    # This is the default vhost for @@SECONDARY_PORT@@
+    # Your request will land here when you pass in an unknown host.
+    listen @@SECONDARY_PORT@@;
+    listen [::]:@@SECONDARY_PORT@@;
+    server_name default.example.com;
+    pagespeed FileCachePath "@@FILE_CACHE@@";
+    pagespeed RewriteLevel CoreFilters;
+  }
+
   server {
     # Sets up a logical home-page server on
     # max-cacheable-content-length.example.com.  This server is only used to