Rainer Weikusat <[email protected]> writes:
> The attached patch moves the body of the timeout handling code from
> the ConnOpener::connect method to the ConnOpener::timeout method
> instead of invoking ::connect from ::timeout in order to make squid
> enforced connect timeouts work
[...]
Attached is a preliminary updated version of this patch, 'preliminary'
here meaning it has survived tests for all use cases I could think of
at the moment without crashing the proxy. Changes:
- deals with the ConnOpener memory leak in ::timeout
- schedules the timeout via eventAdd instead of 'as file descriptor
timeout'
- closes temporaryFd_ before doing a connect retry and opens a new
connection from DelayedConnectRetry
I've left the stillConnecting as is, awaiting future comments on that.
The purpose of this code is mainly to solicit comments as well. I
assume that it is still buggy in subtle and less subtle ways (please
keep in mind that I haven't ever seen any of this before last Sunday
:-}). Also, it contains somewhat more code-duplication than will
likely be in a final version.
--- mad-squid-timeout/src/comm/ConnOpener.cc 8 Jan 2013 17:36:44 -0000
1.1.1.2
+++ mad-squid-timeout/src/comm/ConnOpener.cc 15 Jan 2013 22:10:25 -0000
1.1.1.2.2.10
@@ -139,16 +139,11 @@
if (temporaryFd_ >= 0) {
debugs(5, 4, HERE << conn_ << " closing temp FD " << temporaryFd_);
- // it never reached fully open, so cleanup the FD handlers
- // Note that comm_close() sequence does not happen for partially open
FD
- Comm::SetSelect(temporaryFd_, COMM_SELECT_WRITE, NULL, NULL, 0);
calls_.earlyAbort_ = NULL;
if (calls_.timeout_ != NULL) {
calls_.timeout_->cancel("Comm::ConnOpener::doneConnecting");
calls_.timeout_ = NULL;
}
- fd_table[temporaryFd_].timeoutHandler = NULL;
- fd_table[temporaryFd_].timeout = 0;
close(temporaryFd_);
fd_close(temporaryFd_);
temporaryFd_ = -1;
@@ -192,9 +187,9 @@
typedef CommTimeoutCbParams Params;
Params ¶ms = GetCommParams<Params>(calls_.timeout_);
params.conn = conn_;
- fd_table[temporaryFd_].timeoutHandler = calls_.timeout_;
- fd_table[temporaryFd_].timeout = squid_curtime + (time_t) connectTimeout_;
+ eventAdd("ConnectTimeout", Comm::ConnOpener::ConnectTimeout, new
Pointer(this), connectTimeout_, 0, false);
+
connectStart_ = squid_curtime;
connect();
}
@@ -233,25 +228,16 @@
{
Must(conn_ != NULL);
- // our parent Jobs signal abort by cancelling their callbacks.
- if (callback_ == NULL || callback_->canceled())
- return;
+ if (!stillConnecting())
+ return;
++ totalTries_;
switch (comm_connect_addr(temporaryFd_, conn_->remote) ) {
case COMM_INPROGRESS:
- // check for timeout FIRST.
- if (squid_curtime - connectStart_ > connectTimeout_) {
- debugs(5, 5, HERE << conn_ << ": * - ERR took too long already.");
- calls_.earlyAbort_->cancel("Comm::ConnOpener::connect timed out");
- doneConnecting(COMM_TIMEOUT, errno);
- return;
- } else {
- debugs(5, 5, HERE << conn_ << ": COMM_INPROGRESS");
- Comm::SetSelect(temporaryFd_, COMM_SELECT_WRITE,
Comm::ConnOpener::InProgressConnectRetry, new Pointer(this), 0);
- }
+ debugs(5, 5, HERE << conn_ << ": COMM_INPROGRESS");
+ Comm::SetSelect(temporaryFd_, COMM_SELECT_WRITE,
Comm::ConnOpener::InProgressConnectRetry, new Pointer(this), 0);
break;
case COMM_OK:
@@ -263,12 +249,14 @@
default:
++failRetries_;
- // check for timeout FIRST.
- if (squid_curtime - connectStart_ > connectTimeout_) {
- debugs(5, 5, HERE << conn_ << ": * - ERR took too long to receive
response.");
- calls_.earlyAbort_->cancel("Comm::ConnOpener::connect timed out");
- doneConnecting(COMM_TIMEOUT, errno);
- } else if (failRetries_ < Config.connect_retries) {
+ if (failRetries_ < Config.connect_retries) {
+ comm_remove_close_handler(temporaryFd_, calls_.earlyAbort_);
+ calls_.earlyAbort_ = NULL;
+
+ close(temporaryFd_);
+ fd_close(temporaryFd_);
+ temporaryFd_ = -1;
+
debugs(5, 5, HERE << conn_ << ": * - try again");
eventAdd("Comm::ConnOpener::DelayedConnectRetry",
Comm::ConnOpener::DelayedConnectRetry, new Pointer(this), 0.05, 0, false);
return;
@@ -281,6 +269,16 @@
}
}
+/** Check if the result of a connect request is still undecided.
+ * Used by ::timeout and ::connect to determine if there is
+ * still something to do.
+ */
+bool
+Comm::ConnOpener::stillConnecting()
+{
+ return callback_ != 0 && !callback_->canceled();
+}
+
/**
* Lookup local-end address and port of the TCP link just opened.
* This ensure the connection local details are set correctly
@@ -319,7 +317,24 @@
void
Comm::ConnOpener::timeout(const CommTimeoutCbParams &)
{
- connect();
+ fde *F;
+ Pointer *ptr;
+
+ if (!stillConnecting())
+ return;
+
+ if (temporaryFd_ > -1) {
+ F = &fd_table[temporaryFd_];
+ if (F->write_handler != NULL) {
+ ptr = static_cast<Pointer *>(F->write_data);
+ delete ptr;
+ }
+
+ calls_.earlyAbort_->cancel("Comm::ConnOpener::connect timed out");
+ }
+
+ debugs(5, 5, HERE << conn_ << ": * - ERR took too long to receive
response.");
+ doneConnecting(COMM_TIMEOUT, ETIMEDOUT);
}
/* Legacy Wrapper for the retry event after COMM_INPROGRESS
@@ -349,6 +364,19 @@
Pointer *ptr = static_cast<Pointer*>(data);
assert(ptr);
if (ConnOpener *cs = ptr->valid()) {
+ cs->temporaryFd_ = comm_openex(SOCK_STREAM, IPPROTO_TCP,
+ cs->conn_->local, cs->conn_->flags,
+ cs->conn_->tos, cs->conn_->nfmark,
+ cs->host_);
+ if (cs->temporaryFd_ < 0) {
+ cs->doneConnecting(COMM_ERR_CONNECT, 0);
+ return;
+ }
+
+ typedef CommCbMemFunT<Comm::ConnOpener, CommCloseCbParams> abortDialer;
+ cs->calls_.earlyAbort_ = JobCallback(5, 4, abortDialer, cs,
Comm::ConnOpener::earlyAbort);
+ comm_add_close_handler(cs->temporaryFd_, cs->calls_.earlyAbort_);
+
// Ew. we are now outside the all AsyncJob protections.
// get back inside by scheduling another call...
typedef NullaryMemFunT<Comm::ConnOpener> Dialer;
@@ -357,3 +385,16 @@
}
delete ptr;
}
+
+/* Legacy wrapper for invoking the timeout call via event scheduler.
+ See above for XXX.
+*/
+void
+Comm::ConnOpener::ConnectTimeout(void *data)
+{
+ Pointer *ptr = static_cast<Pointer *>(data);
+ assert(ptr);
+ if (ConnOpener *cs = ptr->valid())
+ ScheduleCallHere(cs->calls_.timeout_);
+ delete ptr;
+}
--- mad-squid-timeout/src/comm/ConnOpener.h 7 Jan 2013 20:15:28 -0000
1.1.1.1
+++ mad-squid-timeout/src/comm/ConnOpener.h 15 Jan 2013 22:15:46 -0000
@@ -43,9 +43,11 @@
void doneConnecting(comm_err_t status, int xerrno);
static void InProgressConnectRetry(int fd, void *data);
static void DelayedConnectRetry(void *data);
+ static void ConnectTimeout(void *data);
void connect();
void connected();
void lookupLocalAddress();
+ bool stillConnecting();
private:
char *host_; ///< domain name we are trying to
connect to.
