From 092da4a8851adefb75262c86465b80ec66deb94b Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Mon, 23 Jan 2017 00:43:44 +1300 Subject: [PATCH] Reduce crashes due to unexpected ClientHttpRequest termination. The underlying problem has been known since r13480: If a ClientHttpRequest job ends without Http::Stream (and ConnStateData) knowledge, then Squid is likely to segfault or assert. This patch does not resolve the underlying issue (a proper fix would require architectural changes in a consensus-lacking area) but makes an unexpected ClientHttpRequest job destruction less likely. BodyPipe and Adaptation-related exceptions are the major causes of unexpected ClientHttpRequest job destruction. This patch handles them by closing the client connection. Connection closure should trigger an orderly top-down cleanup, including Http::Stream, ConnStateData, and ClientHttpRequest destruction. If there is no connection to close, then the exception is essentially ignored with a level-1 error message disclosing the problem. The side effects of ignoring such exceptions are unknown, but without a client connection, it is our hope that they would be relatively benign. This is a Measurement Factory project. --- src/client_side_request.cc | 15 +++++++++++++++ src/client_side_request.h | 1 + 2 files changed, 16 insertions(+) diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 5c3764bfa3..a21ead09dc 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -2069,6 +2069,21 @@ ClientHttpRequest::handleAdaptationFailure(int errDetail, bool bypassable) doCallouts(); } +void +ClientHttpRequest::callException(const std::exception &ex) +{ + if (const auto clientConn = getConn() ? getConn()->clientConnection : nullptr) { + if (Comm::IsConnOpen(clientConn)) { + debugs(85, 3, "closing after exception: " << ex.what()); + clientConn->close(); // initiate orderly top-to-bottom cleanup + return; + } + } + debugs(85, DBG_IMPORTANT, "ClientHttpRequest exception without connection. Ignoring " << ex.what()); + // XXX: Normally, we mustStop() but we cannot do that here because it is + // likely to leave Http::Stream and ConnStateData with a dangling http + // pointer. See r13480 or XXX in Http::Stream class description. +} #endif // XXX: modify and use with ClientRequestContext::clientAccessCheckDone too. diff --git a/src/client_side_request.h b/src/client_side_request.h index a3ce992df4..edecdc4d8e 100644 --- a/src/client_side_request.h +++ b/src/client_side_request.h @@ -121,6 +121,7 @@ public: return Initiator::doneAll() && BodyConsumer::doneAll() && false; } + virtual void callException(const std::exception &ex); #endif private: -- 2.47.3