On 11/01/2017 04:50 μμ, Amos Jeffries wrote:
On 11/01/2017 10:55 p.m., Christos Tsantilas wrote:
We observed such problems on squid shutdown procedure and during regular
squid operation. Any clientStreams redesign should take care of such
problems.
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
+1. Looks like a reasonable temporary fix. Thank you.
The t5 patch which has minor fixes over the posted patch applied to
squid-5 branch as r15004.
The same patch apply as is to squid-4 branch.
I am also attaching the patch for squid-3.5
Amos
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.
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2017-01-01 00:16:45 +0000
+++ src/client_side_request.cc 2017-01-11 18:15:01 +0000
@@ -2063,22 +2063,36 @@
noAddr.setNoAddr();
ConnStateData * c = getConn();
calloutContext->error = clientBuildError(ERR_ICAP_FAILURE, Http::scInternalServerError,
NULL,
c != NULL ? c->clientConnection->remote : noAddr,
request
);
#if USE_AUTH
calloutContext->error->auth_user_request =
c != NULL && c->getAuth() != NULL ? c->getAuth() : request->auth_user_request;
#endif
calloutContext->error->detailError(errDetail);
calloutContext->readNextRequest = true;
if (c != NULL)
c->expectNoForwarding();
doCallouts();
}
//else if(calloutContext == NULL) is it possible?
}
+void
+ClientHttpRequest::callException(const std::exception &ex)
+{
+ const Comm::ConnectionPointer clientConn = getConn() ? getConn()->clientConnection : NULL;
+ 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
=== modified file 'src/client_side_request.h'
--- src/client_side_request.h 2017-01-01 00:16:45 +0000
+++ src/client_side_request.h 2017-01-11 17:53:13 +0000
@@ -96,40 +96,41 @@
} flags;
struct {
Http::StatusCode status;
char *location;
} redirect;
dlink_node active;
dlink_list client_stream;
int mRangeCLen();
ClientRequestContext *calloutContext;
void doCallouts();
#if USE_ADAPTATION
// AsyncJob virtual methods
virtual bool doneAll() const {
return Initiator::doneAll() &&
BodyConsumer::doneAll() && false;
}
+ virtual void callException(const std::exception &ex);
#endif
private:
int64_t maxReplyBodySize_;
StoreEntry *entry_;
StoreEntry *loggingEntry_;
ConnStateData * conn_;
#if USE_OPENSSL
/// whether (and how) the request needs to be bumped
Ssl::BumpMode sslBumpNeed_;
public:
/// returns raw sslBump mode value
Ssl::BumpMode sslBumpNeed() const { return sslBumpNeed_; }
/// returns true if and only if the request needs to be bumped
bool sslBumpNeeded() const { return sslBumpNeed_ == Ssl::bumpServerFirst || sslBumpNeed_ == Ssl::bumpClientFirst || sslBumpNeed_ == Ssl::bumpBump || sslBumpNeed_ == Ssl::bumpPeek || sslBumpNeed_ == Ssl::bumpStare; }
/// set the sslBumpNeeded state
void sslBumpNeed(Ssl::BumpMode mode);
void sslBumpStart();
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.
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2017-01-08 05:12:44 +0000
+++ src/client_side_request.cc 2017-01-11 18:17:47 +0000
@@ -2048,40 +2048,55 @@
if (bypassable && !usedStore && !usedPipe) {
debugs(85,3, HERE << "ICAP REQMOD callout failed, bypassing: " << calloutContext);
if (calloutContext)
doCallouts();
return;
}
debugs(85,3, HERE << "ICAP REQMOD callout failed, responding with error");
clientStreamNode *node = (clientStreamNode *)client_stream.tail->prev->data;
clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
assert(repContext);
calloutsError(ERR_ICAP_FAILURE, errDetail);
if (calloutContext)
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.
void
ClientHttpRequest::calloutsError(const err_type error, const int errDetail)
{
// The original author of the code also wanted to pass an errno to
// setReplyToError, but it seems unlikely that the errno reflects the
// true cause of the error at this point, so I did not pass it.
if (calloutContext) {
Ip::Address noAddr;
noAddr.setNoAddr();
ConnStateData * c = getConn();
calloutContext->error = clientBuildError(error, Http::scInternalServerError,
NULL,
c != NULL ? c->clientConnection->remote : noAddr,
request
);
#if USE_AUTH
calloutContext->error->auth_user_request =
=== modified file 'src/client_side_request.h'
--- src/client_side_request.h 2017-01-08 05:12:44 +0000
+++ src/client_side_request.h 2017-01-11 18:17:47 +0000
@@ -114,40 +114,41 @@
Http::StatusCode status;
char *location;
} redirect;
dlink_node active;
dlink_list client_stream;
int mRangeCLen();
ClientRequestContext *calloutContext;
void doCallouts();
/// Build an error reply. For use with the callouts.
void calloutsError(const err_type error, const int errDetail);
#if USE_ADAPTATION
// AsyncJob virtual methods
virtual bool doneAll() const {
return Initiator::doneAll() &&
BodyConsumer::doneAll() && false;
}
+ virtual void callException(const std::exception &ex);
#endif
private:
int64_t maxReplyBodySize_;
StoreEntry *entry_;
StoreEntry *loggingEntry_;
ConnStateData * conn_;
#if USE_OPENSSL
/// whether (and how) the request needs to be bumped
Ssl::BumpMode sslBumpNeed_;
public:
/// returns raw sslBump mode value
Ssl::BumpMode sslBumpNeed() const { return sslBumpNeed_; }
/// returns true if and only if the request needs to be bumped
bool sslBumpNeeded() const { return sslBumpNeed_ == Ssl::bumpServerFirst || sslBumpNeed_ == Ssl::bumpClientFirst || sslBumpNeed_ == Ssl::bumpBump || sslBumpNeed_ == Ssl::bumpPeek || sslBumpNeed_ == Ssl::bumpStare; }
/// set the sslBumpNeeded state
void sslBumpNeed(Ssl::BumpMode mode);
void sslBumpStart();
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
