Log %err_code for ERR_RELAY_REMOTE transactions (#1472)

For ERR_RELAY_REMOTE transactions, Squid was logging %err_code as "-"
because BuildHttpReply() was not updating HttpRequest::error or ALE.
That update was missing because the pre-computed response in those
transactions triggered a premature exit from BuildHttpReply().

BuildHttpReply() should not be updating errors at all, but significant
code refactoring required to fix that problem needs a dedicated change.

Also enabled regression testing for the fixed bug and the bug fixed in
recent commit ea3f56e.

diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc
index f31ce525483b8fb17b645bf349b143a903901e49..1df7b15b0c9e52f0ff1013ea401534681bfea055 100644 (file)
--- a/src/clients/HttpTunneler.cc
+++ b/src/clients/HttpTunneler.cc
@@ -339,7 +339,7 @@ Http::Tunneler::bailOnResponseError(const char *error, HttpReply *errorReply)
 
     ErrorState *err;
     if (errorReply) {
-        err = new ErrorState(request.getRaw(), errorReply);
+        err = new ErrorState(request.getRaw(), errorReply, al);
     } else {
         // with no reply suitable for relaying, answer with 502 (Bad Gateway)
         err = new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al);

diff --git a/src/errorpage.cc b/src/errorpage.cc
index 8200e28f7f673bab95767a61d5a0365fba6e6caa..6a46b7116f539291963da13af21aab9cebb9108b 100644 (file)
--- a/src/errorpage.cc
+++ b/src/errorpage.cc
@@ -678,15 +678,16 @@ ErrorState::NewForwarding(err_type type, HttpRequestPointer &request, const Acce
     return new ErrorState(type, status, request.getRaw(), ale);
 }
 
-ErrorState::ErrorState(err_type t) :
+ErrorState::ErrorState(const err_type t, const AccessLogEntry::Pointer &anAle):
     type(t),
     page_id(t),
-    callback(nullptr)
+    callback(nullptr),
+    ale(anAle)
 {
 }
 
 ErrorState::ErrorState(err_type t, Http::StatusCode status, HttpRequest * req, const AccessLogEntry::Pointer &anAle) :
-    ErrorState(t)
+    ErrorState(t, anAle)
 {
     if (page_id >= ERR_MAX && ErrorDynamicPages[page_id - ERR_MAX]->page_redirect != Http::scNone)
         httpStatus = ErrorDynamicPages[page_id - ERR_MAX]->page_redirect;
@@ -697,12 +698,10 @@ ErrorState::ErrorState(err_type t, Http::StatusCode status, HttpRequest * req, c
         request = req;
         src_addr = req->client_addr;
     }
-
-    ale = anAle;
 }
 
-ErrorState::ErrorState(HttpRequest * req, HttpReply *errorReply) :
-    ErrorState(ERR_RELAY_REMOTE)
+ErrorState::ErrorState(HttpRequest * req, HttpReply *errorReply, const AccessLogEntry::Pointer &anAle):
+    ErrorState(ERR_RELAY_REMOTE, anAle)
 {
     Must(errorReply);
     response_ = errorReply;
@@ -1277,6 +1276,17 @@ ErrorState::validate()
 HttpReply *
 ErrorState::BuildHttpReply()
 {
+    // Make sure error codes get back to the client side for logging and
+    // error tracking.
+    if (request) {
+        request->error.update(type, detail);
+        request->error.update(SysErrorDetail::NewIfAny(xerrno));
+    } else if (ale) {
+        Error err(type, detail);
+        err.update(SysErrorDetail::NewIfAny(xerrno));
+        ale->updateError(err);
+    }
+
     if (response_)
         return response_.getRaw();
 
@@ -1345,17 +1355,6 @@ ErrorState::BuildHttpReply()
         rep->body.set(body);
     }
 
-    // Make sure error codes get back to the client side for logging and
-    // error tracking.
-    if (request) {
-        request->error.update(type, detail);
-        request->error.update(SysErrorDetail::NewIfAny(xerrno));
-    } else if (ale) {
-        Error err(type, detail);
-        err.update(SysErrorDetail::NewIfAny(xerrno));
-        ale->updateError(err);
-    }
-
     return rep;
 }
 

diff --git a/src/errorpage.h b/src/errorpage.h
index 79dea859092c8d4d337741cae9d34a9e696dfdbe..8eb295f8ffa81f7afa519c2a8826c67e433eb094 100644 (file)
--- a/src/errorpage.h
+++ b/src/errorpage.h
@@ -95,7 +95,7 @@ public:
     ErrorState() = delete; // not implemented.
 
     /// creates an ERR_RELAY_REMOTE error
-    ErrorState(HttpRequest * request, HttpReply *);
+    ErrorState(HttpRequest * request, HttpReply *, const AccessLogEntryPointer &);
 
     ~ErrorState();
 
@@ -120,7 +120,7 @@ private:
     typedef ErrorPage::Build Build;
 
     /// initializations shared by public constructors
-    explicit ErrorState(err_type type);
+    ErrorState(err_type, const AccessLogEntryPointer &);
 
     /// locates the right error page template for this error and compiles it
     SBuf buildBody();

diff --git a/test-suite/test-functionality.sh b/test-suite/test-functionality.sh
index ffbabfa37cdb6e968477db2c01037e8d3b59bf42..eb191bc0cc94b10f394d04d07ff20ab2d5fda3a9 100755 (executable)
--- a/test-suite/test-functionality.sh
+++ b/test-suite/test-functionality.sh
@@ -214,6 +214,7 @@ main() {
     then
         local default_tests="
             pconn
+            dead-peer
             proxy-update-headers-after-304
             accumulate-headers-after-304
             upgrade-protocols