Fix %err_code/%err_detail for some error:transaction-end-before-headers

src/client_side.cc

@@ -594,8 +594,7 @@ ConnStateData::swanSong()
     flags.readMore = false;
     clientdbEstablished(clientConnection->remote, -1);  /* decrement */
 
-    terminateAll(ERR_NONE, LogTagsErrors());
-    checkLogging();
+    terminateAll(ERR_STREAM_FAILURE, LogTagsErrors());
 
     // XXX: Closing pinned conn is too harsh: The Client may want to continue!
     unpinConnection(true);
@@ -3904,63 +3903,59 @@ ConnStateData::unpinConnection(const bool andClose)
 void
 ConnStateData::terminateAll(const Error &rawError, const LogTagsErrors &lte)
 {
+    Assure(rawError);
+
     auto error = rawError; // (cheap) copy so that we can detail
-    // We detail even ERR_NONE: There should be no transactions left, and
-    // detailed ERR_NONE will be unused. Otherwise, this detail helps in triage.
     if (error.details.empty()) {
         static const auto d = MakeNamedErrorDetail("WITH_CLIENT");
         error.details.push_back(d);
     }
 
     debugs(33, 3, pipeline.count() << '/' << pipeline.nrequests << " after " << error);
 
-    if (pipeline.empty()) {
-        bareError.update(error); // XXX: bareLogTagsErrors
-    } else {
-        // We terminate the current CONNECT/PUT/etc. context below, logging any
-        // error details, but that context may leave unparsed bytes behind.
-        // Consume them to stop checkLogging() from logging them again later.
-        const auto intputToConsume =
-#if USE_OPENSSL
-            parsingTlsHandshake ? "TLS handshake" : // more specific than CONNECT
-#endif
-            bodyPipe ? "HTTP request body" :
-            pipeline.back()->mayUseConnection() ? "HTTP CONNECT" :
-            nullptr;
-
-        while (const auto context = pipeline.front()) {
-            context->noteIoError(error, lte);
-            context->finished(); // cleanup and self-deregister
-            assert(context != pipeline.front());
-        }
+    const auto leftovers = handleLeftovers();
+
+    while (const auto context = pipeline.front()) {
+        context->noteIoError(error, lte);
+        context->finished(); // cleanup and self-deregister
+        assert(context != pipeline.front());
+    }
 
-        if (intputToConsume && !inBuf.isEmpty()) {
-            debugs(83, 5, "forgetting client " << intputToConsume << " bytes: " << inBuf.length());
-            inBuf.clear();
+    if (!terminatedAll) {
+        if (leftovers || !pipeline.nrequests) {
+            /* Create a temporary ClientHttpRequest object. Its destructor will log. */
+            ClientHttpRequest http(this);
+            http.req_sz = inBuf.length();
+            // XXX: Or we died while waiting for the pinned connection to become idle.
+            http.setErrorUri("error:transaction-end-before-headers");
+            http.updateError(error);
         }
     }
 
+    terminatedAll = true;
+
     clientConnection->close();
 }
 
-/// log the last (attempt at) transaction if nobody else did
-void
-ConnStateData::checkLogging()
+/// whether we should ignore unparsed inBuf bytes
+/// during transaction termination
+bool
+ConnStateData::handleLeftovers()
 {
-    // to simplify our logic, we assume that terminateAll() has been called
-    assert(pipeline.empty());
+    const auto intputToConsume =
+#if USE_OPENSSL
+        parsingTlsHandshake ? "TLS handshake" : // more specific than CONNECT
+#endif
+        bodyPipe ? "HTTP request body" :
+        (!pipeline.empty() && pipeline.back()->mayUseConnection()) ? "HTTP CONNECT" :
+        nullptr;
 
-    // do not log connections that closed after a transaction (it is normal)
-    // TODO: access_log needs ACLs to match received-no-bytes connections
-    if (pipeline.nrequests && inBuf.isEmpty())
-        return;
+    if (intputToConsume && !inBuf.isEmpty()) {
+        debugs(83, 5, "forgetting client " << intputToConsume << " bytes: " << inBuf.length());
+        inBuf.clear();
+    }
 
-    /* Create a temporary ClientHttpRequest object. Its destructor will log. */
-    ClientHttpRequest http(this);
-    http.req_sz = inBuf.length();
-    // XXX: Or we died while waiting for the pinned connection to become idle.
-    http.setErrorUri("error:transaction-end-before-headers");
-    http.updateError(bareError);
+    return !inBuf.isEmpty();
 }
 
 bool

src/client_side.h

@@ -440,7 +440,7 @@ class ConnStateData:
     void terminateAll(const Error &, const LogTagsErrors &) override;
     bool shouldCloseOnEof() const override;
 
-    void checkLogging();
+    bool handleLeftovers();
 
     void parseRequests();
     void clientAfterReadingRequests();
@@ -503,6 +503,8 @@ class ConnStateData:
     /// If set, are propagated to the current and all future master transactions
     /// on the connection.
     NotePairs::Pointer theNotes;
+    /// whether terminateAll() has been called
+    bool terminatedAll = false;
 };
 
 const char *findTrailingHTTPVersion(const char *uriAndHTTPVersion, const char *end = nullptr);

src/error/forward.h

@@ -81,6 +81,7 @@ typedef enum {
     ERR_REQUEST_START_TIMEOUT, // Aborts the connection instead of error page
     ERR_REQUEST_PARSE_TIMEOUT, // Aborts the connection instead of error page
     ERR_RELAY_REMOTE, // Sends server reply instead of error page
+    ERR_STREAM_FAILURE, // No client to send the error page to
 
     /* Cache Manager GUI can install a manager index/home page */
     MGR_INDEX,