From: Amos Jeffries <squid3@treenet.co.nz>
Date: Sun, 27 Jun 2010 11:31:31 +0000 (+1200)
Subject: Audit round 2 fallout.
X-Git-Tag: take08~55^2~124^2~127
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=aed188fdd4501cffaedab8767e873b66d77a2f99;p=thirdparty%2Fsquid.git

Audit round 2 fallout.

Main changes:

 * removes multi-destination connection ability from ConnectStateData

 * renames ConnectSateData to ConnOpener (now that its only purpose is
    to open a given single connection

 * initial stab at making ConnOpener an AsyncJob
---

diff --git a/src/CommCalls.cc b/src/CommCalls.cc
index 73a65daf91..da1754e04e 100644
--- a/src/CommCalls.cc
+++ b/src/CommCalls.cc
@@ -75,9 +75,6 @@ CommConnectCbParams::print(std::ostream &os) const
     CommCommonCbParams::print(os);
     if (conn != NULL)
       os << ", from my " << conn->local << " to " << conn->remote;
-    else if (paths && paths->size() > 0) {
-        // TODO: for each path. print the to => from path being attempted.
-    }
 }
 
 /* CommIoCbParams */
@@ -163,7 +160,7 @@ CommConnectCbPtrFun::CommConnectCbPtrFun(CNCB *aHandler,
 void
 CommConnectCbPtrFun::dial()
 {
-    handler(params.conn, params.paths, params.flag, params.xerrno, params.data);
+    handler(params.conn, params.flag, params.xerrno, params.data);
 }
 
 void
diff --git a/src/CommCalls.h b/src/CommCalls.h
index c78c8e6e87..5222557104 100644
--- a/src/CommCalls.h
+++ b/src/CommCalls.h
@@ -21,8 +21,8 @@
  *     - I/O (IOCB).
  */
 
-typedef void IOACB(int fd, int nfd, Comm::ConnectionPointer details, comm_err_t flag, int xerrno, void *data);
-typedef void CNCB(Comm::ConnectionPointer conn, Comm::PathsPointer paths, comm_err_t status, int xerrno, void *data);
+typedef void IOACB(int fd, int nfd, Comm::ConnectionPointer &details, comm_err_t flag, int xerrno, void *data);
+typedef void CNCB(Comm::ConnectionPointer &conn, comm_err_t status, int xerrno, void *data);
 typedef void IOCB(int fd, char *, size_t size, comm_err_t flag, int xerrno, void *data);
 
 /*
@@ -88,7 +88,6 @@ public:
 
 public:
     Comm::ConnectionPointer conn;
-    Comm::PathsPointer paths;
 };
 
 // read/write (I/O) parameters
diff --git a/src/PeerSelectState.h b/src/PeerSelectState.h
index a84aa6e7c9..c25cf33044 100644
--- a/src/PeerSelectState.h
+++ b/src/PeerSelectState.h
@@ -43,9 +43,9 @@
 class HttpRequest;
 class StoreEntry;
 
-typedef void PSC(Comm::PathsPointer, void *);
+typedef void PSC(Comm::Paths *, void *);
 
-SQUIDCEXTERN void peerSelect(Comm::PathsPointer, HttpRequest *, StoreEntry *, PSC *, void *data);
+SQUIDCEXTERN void peerSelect(Comm::Paths *, HttpRequest *, StoreEntry *, PSC *, void *data);
 SQUIDCEXTERN void peerSelectInit(void);
 
 /**
@@ -79,8 +79,8 @@ public:
     PSC *callback;
     void *callback_data;
 
-    Comm::PathsPointer paths;   ///< the callers paths array. to be filled with our final results.
-    FwdServer *servers;       	///< temporary linked list of peers we will pass back.
+    Comm::Paths *paths;    ///< the callers paths array. to be filled with our final results.
+    FwdServer *servers;    ///< temporary linked list of peers we will pass back.
 
     /*
      * Why are these Ip::Address instead of peer *?  Because a
@@ -105,5 +105,4 @@ private:
     CBDATA_CLASS(ps_state);
 };
 
-
 #endif /* SQUID_PEERSELECTSTATE_H */
diff --git a/src/adaptation/icap/ModXact.cc b/src/adaptation/icap/ModXact.cc
index 1b47fd91bf..bfc5d58c6b 100644
--- a/src/adaptation/icap/ModXact.cc
+++ b/src/adaptation/icap/ModXact.cc
@@ -464,7 +464,7 @@ bool Adaptation::Icap::ModXact::doneAll() const
 
 void Adaptation::Icap::ModXact::startReading()
 {
-    Must(connection != NULL && connection->isOpen());
+    Must(haveConnection());
     Must(!reader);
     Must(!adapted.header);
     Must(!adapted.body_pipe);
@@ -622,7 +622,7 @@ void Adaptation::Icap::ModXact::bypassFailure()
     stopParsing();
 
     stopWriting(true); // or should we force it?
-    if (connection != NULL && connection->isOpen()) {
+    if (haveConnection()) {
         reuseConnection = false; // be conservative
         cancelRead(); // may not work; and we cannot stop connecting either
         if (!doneWithIo())
@@ -1442,7 +1442,7 @@ void Adaptation::Icap::ModXact::fillPendingStatus(MemBuf &buf) const
     if (virgin.body_pipe != NULL)
         buf.append("R", 1);
 
-    if (connection != NULL && connection->isOpen() && !doneReading())
+    if (haveConnection() && !doneReading())
         buf.append("r", 1);
 
     if (!state.doneWriting() && state.writing != State::writingInit)
diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc
index 4d5bed63c3..8f7d7bec40 100644
--- a/src/adaptation/icap/Xaction.cc
+++ b/src/adaptation/icap/Xaction.cc
@@ -5,7 +5,7 @@
 #include "squid.h"
 #include "comm.h"
 #include "comm/Connection.h"
-#include "comm/ConnectStateData.h"
+#include "comm/ConnOpener.h"
 #include "CommCalls.h"
 #include "HttpMsg.h"
 #include "adaptation/icap/Xaction.h"
@@ -91,7 +91,7 @@ void Adaptation::Icap::Xaction::openConnection()
 {
     Ip::Address client_addr;
 
-    Must(connection == NULL || !connection->isOpen());
+    Must(!haveConnection());
 
     const Adaptation::Service &s = service();
 
@@ -102,8 +102,7 @@ void Adaptation::Icap::Xaction::openConnection()
 
     /* NP: set these here because it applies whether a pconn or a new conn is used */
 
-    // TODO:  where do we get the DNS info for the ICAP server host ??
-    //        Ip::Address will do a BLOCKING lookup if s.cfg().host is a hostname
+    // TODO: Avoid blocking lookup if s.cfg().host is a hostname
     connection->remote = s.cfg().host.termedBuf();
     connection->remote.SetPort(s.cfg().port);
 
@@ -130,10 +129,10 @@ void Adaptation::Icap::Xaction::openConnection()
     connector = asyncCall(93,3, "Adaptation::Icap::Xaction::noteCommConnected",
                           ConnectDialer(this, &Adaptation::Icap::Xaction::noteCommConnected));
 
-    ConnectStateData *cs = new ConnectStateData(connection, connector);
-    cs->host = xstrdup(s.cfg().host.termedBuf());
+    ConnOpener *cs = new ConnOpener(connection, connector);
+    cs->setHost(s.cfg().host.termedBuf());
     cs->connect_timeout = TheConfig.connect_timeout(service().cfg().bypass);
-    cs->connect();
+    cs->start();
 }
 
 /*
@@ -152,7 +151,7 @@ Adaptation::Icap::Xaction::reusedConnection(void *data)
 
 void Adaptation::Icap::Xaction::closeConnection()
 {
-    if (connection != NULL && connection->isOpen()) {
+    if (haveConnection()) {
 
         if (closer != NULL) {
             comm_remove_close_handler(connection->fd, closer);
@@ -211,7 +210,6 @@ void Adaptation::Icap::Xaction::noteCommConnected(const CommConnectCbParams &io)
 
     fd_table[io.conn->fd].noteUse(icapPconnPool);
 
-    connection = io.conn;
     handleCommConnected();
 }
 
@@ -224,6 +222,8 @@ void Adaptation::Icap::Xaction::dieOnConnectionFailure()
 
 void Adaptation::Icap::Xaction::scheduleWrite(MemBuf &buf)
 {
+    Must(haveConnection());
+
     // comm module will free the buffer
     typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommIoCbParams> Dialer;
     writer = asyncCall(93,3, "Adaptation::Icap::Xaction::noteCommWrote",
@@ -303,6 +303,8 @@ bool Adaptation::Icap::Xaction::doneAll() const
 
 void Adaptation::Icap::Xaction::updateTimeout()
 {
+    Must(haveConnection());
+
     if (reader != NULL || writer != NULL) {
         // restart the timeout before each I/O
         // XXX: why does Config.Timeout lacks a write timeout?
@@ -323,7 +325,7 @@ void Adaptation::Icap::Xaction::updateTimeout()
 
 void Adaptation::Icap::Xaction::scheduleRead()
 {
-    Must(connection->isOpen());
+    Must(haveConnection());
     Must(!reader);
     Must(readBuf.hasSpace());
 
@@ -373,6 +375,7 @@ void Adaptation::Icap::Xaction::noteCommRead(const CommIoCbParams &io)
 void Adaptation::Icap::Xaction::cancelRead()
 {
     if (reader != NULL) {
+        Must(haveConnection());
         comm_read_cancel(connection->fd, reader);
         reader = NULL;
     }
@@ -414,11 +417,16 @@ bool Adaptation::Icap::Xaction::doneWriting() const
 
 bool Adaptation::Icap::Xaction::doneWithIo() const
 {
-    return connection != NULL && connection->isOpen() && // or we could still be waiting to open it
+    return haveConnection() &&
            !connector && !reader && !writer && // fast checks, some redundant
            doneReading() && doneWriting();
 }
 
+bool Adaptation::Icap::Xaction::haveConnection() const
+{
+    return connection != NULL && connection->isOpen();
+}
+
 // initiator aborted
 void Adaptation::Icap::Xaction::noteInitiatorAborted()
 {
@@ -529,7 +537,7 @@ const char *Adaptation::Icap::Xaction::status() const
 
 void Adaptation::Icap::Xaction::fillPendingStatus(MemBuf &buf) const
 {
-    if (connection->isOpen()) {
+    if (haveConnection()) {
         buf.Printf("FD %d", connection->fd);
 
         if (writer != NULL)
@@ -544,7 +552,7 @@ void Adaptation::Icap::Xaction::fillPendingStatus(MemBuf &buf) const
 
 void Adaptation::Icap::Xaction::fillDoneStatus(MemBuf &buf) const
 {
-    if (connection->isOpen() && commEof)
+    if (haveConnection() && commEof)
         buf.Printf("Comm(%d)", connection->fd);
 
     if (stopReason != NULL)
diff --git a/src/adaptation/icap/Xaction.h b/src/adaptation/icap/Xaction.h
index 1f0d10370d..988660b570 100644
--- a/src/adaptation/icap/Xaction.h
+++ b/src/adaptation/icap/Xaction.h
@@ -99,6 +99,7 @@ protected:
     void openConnection();
     void closeConnection();
     void dieOnConnectionFailure();
+    bool haveConnection() const;
 
     void scheduleRead();
     void scheduleWrite(MemBuf &buf);
@@ -140,7 +141,7 @@ private:
     void maybeLog();
 
 protected:
-    Comm::ConnectionPointer connection;     // Handle to the ICAP server connection
+    Comm::ConnectionPointer connection;     ///< ICAP server connection
     Adaptation::Icap::ServiceRep::Pointer theService;
 
     /*
diff --git a/src/auth/User.cc b/src/auth/User.cc
index 31e11c3634..0e6e4effc5 100644
--- a/src/auth/User.cc
+++ b/src/auth/User.cc
@@ -53,7 +53,7 @@ CBDATA_TYPE(AuthUserIP);
 
 time_t AuthUser::last_discard = 0;
 
-char *CredentialsState_str[] = { "Unchecked", "Ok", "Pending", "Handshake", "Failed" };
+const char *CredentialsState_str[] = { "Unchecked", "Ok", "Pending", "Handshake", "Failed" };
 
 
 AuthUser::AuthUser(AuthConfig *aConfig) :
diff --git a/src/auth/User.h b/src/auth/User.h
index 1a56933590..ba00233ab4 100644
--- a/src/auth/User.h
+++ b/src/auth/User.h
@@ -128,7 +128,7 @@ private:
     dlink_list ip_list;
 };
 
-extern char *CredentialsState_str[];
+extern const char *CredentialsState_str[];
 
 #if _USE_INLINE_
 #include "auth/User.cci"
diff --git a/src/cf.data.pre b/src/cf.data.pre
index c8af4a6856..1d0ed66692 100644
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -6793,8 +6793,9 @@ TYPE: int
 LOC: Config.connect_retries
 DEFAULT: 0
 DOC_START
-	This sets the maximum number of connection attempts for each
-	potential host address selected by forwarding.
+	This sets the maximum number of connection attempts made for each
+	TCP connection. The connect_retries attempts must all still
+	complete within the connection timeout period.
 
 	The default is not to re-try if the first connection attempt fails.
 	The (not recommended) maximum is 10 tries.
@@ -6805,9 +6806,6 @@ DOC_START
 	Note: These re-tries are in addition to forward_max_tries
 	which limit how many different addresses may be tried to find
 	a useful server.
-
-	The connect_retries * forward_max_tries attempts must all still
-	complete within the connection timeout period.
 DOC_END
 
 NAME: retry_on_error
diff --git a/src/client_side.cc b/src/client_side.cc
index 301d017ede..70c2f7ed88 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3054,7 +3054,7 @@ connStateCreate(const Ip::Address &peer, const Ip::Address &me, int fd, http_por
 
 /** Handle a new connection on HTTP socket. */
 void
-httpAccept(int sock, int newfd, Comm::ConnectionPointer details,
+httpAccept(int sock, int newfd, Comm::ConnectionPointer &details,
            comm_err_t flag, int xerrno, void *data)
 {
     http_port_list *s = (http_port_list *)data;
diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc
new file mode 100644
index 0000000000..06cf7a3b9c
--- /dev/null
+++ b/src/comm/ConnOpener.cc
@@ -0,0 +1,182 @@
+#include "config.h"
+#include "comm/ConnOpener.h"
+#include "comm/Connection.h"
+#include "comm.h"
+#include "CommCalls.h"
+#include "fde.h"
+#include "icmp/net_db.h"
+#include "SquidTime.h"
+
+CBDATA_CLASS_INIT(ConnOpener);
+
+ConnOpener::ConnOpener(Comm::ConnectionPointer &c, AsyncCall::Pointer handler) :
+        AsyncJob("ConnOpener"),
+        connect_timeout(Config.Timeout.connect),
+        host(NULL),
+        solo(c),
+        callback(handler),
+        total_tries(0),
+        fail_retries(0),
+        connstart(0)
+{}
+
+ConnOpener::~ConnOpener()
+{
+    safe_free(host);
+    solo = NULL;
+}
+
+void
+ConnOpener::setHost(const char * new_host)
+{
+    if (host == NULL) {
+        safe_free(host); // unset and erase if already set.
+    } else {
+        host = xstrdup(new_host);
+    }
+}
+
+const char *
+ConnOpener::getHost() const
+{
+    return host;
+}
+
+void
+ConnOpener::callCallback(comm_err_t status, int xerrno)
+{
+    /* remove handlers we don't want to happen now */
+    comm_remove_close_handler(solo->fd, ConnOpener::EarlyAbort, this);
+    commSetTimeout(solo->fd, -1, NULL, NULL);
+
+    typedef CommConnectCbParams Params;
+    Params &params = GetCommParams<Params>(callback);
+    params.conn = solo;
+    params.flag = status;
+    params.xerrno = xerrno;
+    ScheduleCallHere(callback);
+
+    callback = NULL;
+    delete this;
+}
+
+void
+ConnOpener::start()
+{
+    /* handle connecting to one single path */
+    if (solo->fd < 0) {
+#if USE_IPV6
+        /* outbound sockets have no need to be protocol agnostic. */
+        if (solo->local.IsIPv6() && solo->local.IsIPv4()) {
+            solo->local.SetIPv4();
+        }
+#endif
+        solo->fd = comm_openex(SOCK_STREAM, IPPROTO_TCP, solo->local, solo->flags, solo->tos, host);
+        if (solo->fd <= 0) {
+            callCallback(COMM_ERR_CONNECT, 0);
+            return;
+        }
+
+        AsyncCall::Pointer ea_call = commCbCall(5,4, "ConnOpener::EarlyAbort",
+                                                CommCloseCbPtrFun(ConnOpener::EarlyAbort, this));
+        comm_add_close_handler(solo->fd, ea_call);
+
+        AsyncCall::Pointer timeout_call = commCbCall(5,4, "ConnOpener::ConnectTimeout",
+                                                     CommTimeoutCbPtrFun(ConnOpener::ConnectTimeout, this));
+        debugs(5, 3, HERE << "FD " << solo->fd << " timeout " << connect_timeout);
+        commSetTimeout(solo->fd, connect_timeout, timeout_call);
+
+        if (connstart == 0) {
+            connstart = squid_curtime;
+        }
+    }
+
+    total_tries++;
+
+    switch (comm_connect_addr(solo->fd, solo->remote) ) {
+
+    case COMM_INPROGRESS:
+        debugs(5, 5, HERE << "FD " << solo->fd << ": COMM_INPROGRESS");
+        commSetSelect(solo->fd, COMM_SELECT_WRITE, ConnOpener::ConnectRetry, this, 0);
+        break;
+
+    case COMM_OK:
+        debugs(5, 5, HERE << "FD " << solo->fd << ": COMM_OK - connected");
+
+        /*
+         * stats.conn_open is used to account for the number of
+         * connections that we have open to the peer, so we can limit
+         * based on the max-conn option.  We need to increment here,
+         * even if the connection may fail.
+         */
+        if (solo->getPeer())
+            solo->getPeer()->stats.conn_open++;
+
+        /* TODO: remove these fd_table accesses. But old code still depends on fd_table flags to
+         *       indicate the state of a raw fd object being passed around.
+         */
+        fd_table[solo->fd].flags.open = 1;
+        solo->local.SetPort(comm_local_port(solo->fd));
+
+        ipcacheMarkGoodAddr(host, solo->remote);
+        callCallback(COMM_OK, 0);
+        break;
+
+    default:
+        debugs(5, 5, HERE "FD " << solo->fd << ": * - try again");
+        fail_retries++;
+        ipcacheMarkBadAddr(host, solo->remote);
+#if USE_ICMP
+        if (Config.onoff.test_reachability)
+            netdbDeleteAddrNetwork(solo->remote);
+#endif
+
+        // check for timeout FIRST.
+        if(squid_curtime - connstart > connect_timeout) {
+            debugs(5, 5, HERE << "FD " << solo->fd << ": * - ERR took too long already.");
+            callCallback(COMM_TIMEOUT, errno);
+        } else if (fail_retries < Config.connect_retries) {
+            start();
+        } else {
+            // send ERROR back to the upper layer.
+            debugs(5, 5, HERE << "FD " << solo->fd << ": * - ERR tried too many times already.");
+            callCallback(COMM_ERR_CONNECT, errno);
+        }
+    }
+}
+
+void
+ConnOpener::EarlyAbort(int fd, void *data)
+{
+    ConnOpener *cs = static_cast<ConnOpener *>(data);
+    debugs(5, 3, HERE << "FD " << fd);
+    cs->callCallback(COMM_ERR_CLOSING, errno); // NP: is closing or shutdown better?
+
+    /* TODO split cases:
+     * remote end rejecting the connection is normal and one of the other paths may be taken.
+     * squid shutting down or forcing abort on the connection attempt(s) are the only real fatal cases.
+     * we may need separate error codes to send back for these two.
+     */
+}
+
+void
+ConnOpener::Connect(void *data)
+{
+    ConnOpener *cs = static_cast<ConnOpener *>(data);
+    cs->start();
+}
+
+void
+ConnOpener::ConnectRetry(int fd, void *data)
+{
+    ConnOpener *cs = static_cast<ConnOpener *>(data);
+    cs->start();
+}
+
+void
+ConnOpener::ConnectTimeout(int fd, void *data)
+{
+    ConnOpener *cs = static_cast<ConnOpener *>(data);
+    cs->start();
+}
+
diff --git a/src/comm/ConnectStateData.h b/src/comm/ConnOpener.h
similarity index 60%
rename from src/comm/ConnectStateData.h
rename to src/comm/ConnOpener.h
index bfdce43cfb..9c4e2a6cc7 100644
--- a/src/comm/ConnectStateData.h
+++ b/src/comm/ConnOpener.h
@@ -1,36 +1,31 @@
-#ifndef _SQUID_SRC_COMM_CONNECTSTATEDATA_H
-#define _SQUID_SRC_COMM_CONNECTSTATEDATA_H
+#ifndef _SQUID_SRC_COMM_OPENERSTATEDATA_H
+#define _SQUID_SRC_COMM_OPENERSTATEDATA_H
 
 #include "base/AsyncCall.h"
+#include "base/AsyncJob.h"
 #include "cbdata.h"
 #include "comm/comm_err_t.h"
 #include "comm/forward.h"
 
 /**
- * State engine handling the opening of a remote outbound connection
- * to one of multiple destinations.
+ * Async-opener of a Comm connection.
  */
-class ConnectStateData
+class ConnOpener : public AsyncJob
 {
 public:
-    /** open first working of a set of connections */
-    ConnectStateData(Comm::PathsPointer paths, AsyncCall::Pointer handler);
+    /** attempt to open a connection. */
+    ConnOpener(Comm::ConnectionPointer &, AsyncCall::Pointer handler);
 
-    /** attempt to open one connection. */
-    ConnectStateData(Comm::ConnectionPointer, AsyncCall::Pointer handler);
+    ~ConnOpener();
 
-    ~ConnectStateData();
-
-    /**
-     * Actual connect start function.
-     */
-    void connect();
+    /** Actual start opening a TCP connection. */
+    void start();
 
 private:
     /* These objects may NOT be created without connections to act on. Do not define this operator. */
-    ConnectStateData();
+    ConnOpener(const ConnOpener &);
     /* These objects may NOT be copied. Do not define this operator. */
-    const ConnectStateData operator =(const ConnectStateData &c);
+    ConnOpener operator =(const ConnOpener &c);
 
     /**
      * Wrapper to start the connection attempts happening.
@@ -46,6 +41,12 @@ private:
      */
     static void EarlyAbort(int fd, void *data);
 
+    /**
+     * Temporary timeout handler used during connect.
+     * Handles the case(s) when a partially setup connection gets timed out.
+     */
+    static void ConnectTimeout(int fd, void *data);
+
     /**
      * Connection attempt are completed. One way or the other.
      * Pass the results back to the external handler.
@@ -53,16 +54,18 @@ private:
     void callCallback(comm_err_t status, int xerrno);
 
 public:
-    char *host;                   ///< domain name we are trying to connect to.
-
     /**
      * time at which to abandon the connection.
      * the connection-done callback will be passed COMM_TIMEOUT
      */
     time_t connect_timeout;
 
+    void setHost(const char *);        ///< set the hostname note for this connection
+    const char * getHost(void) const;  ///< get the hostname noted for this connection
+
 private:
-    Comm::PathsPointer paths;           ///< forwarding paths to be tried. front of the list is the current being opened.
+    char *host;                         ///< domain name we are trying to connect to.
+
     Comm::ConnectionPointer solo;       ///< single connection currently being opened.
     AsyncCall::Pointer callback;        ///< handler to be called on connection completion.
 
@@ -70,7 +73,7 @@ private:
     int fail_retries;  ///< number of retries current destination has been tried.
     time_t connstart;  ///< time at which this series of connection attempts was started.
 
-    CBDATA_CLASS2(ConnectStateData);
+    CBDATA_CLASS2(ConnOpener);
 };
 
-#endif /* _SQUID_SRC_COMM_CONNECTSTATEDATA_H */
+#endif /* _SQUID_SRC_COMM_CONNOPENER_H */
diff --git a/src/comm/ConnectStateData.cc b/src/comm/ConnectStateData.cc
deleted file mode 100644
index bccaf1e505..0000000000
--- a/src/comm/ConnectStateData.cc
+++ /dev/null
@@ -1,205 +0,0 @@
-#include "config.h"
-#include "comm/ConnectStateData.h"
-#include "comm/Connection.h"
-#include "comm.h"
-#include "CommCalls.h"
-#include "fde.h"
-#include "icmp/net_db.h"
-#include "SquidTime.h"
-
-CBDATA_CLASS_INIT(ConnectStateData);
-
-ConnectStateData::ConnectStateData(Comm::PathsPointer paths, AsyncCall::Pointer handler) :
-        host(NULL),
-        connect_timeout(Config.Timeout.connect),
-        paths(paths),
-        solo(NULL),
-        callback(handler),
-        total_tries(0),
-        fail_retries(0),
-        connstart(0)
-{}
-
-ConnectStateData::ConnectStateData(Comm::ConnectionPointer c, AsyncCall::Pointer handler) :
-        host(NULL),
-        connect_timeout(Config.Timeout.connect),
-        paths(NULL),
-        solo(c),
-        callback(handler),
-        total_tries(0),
-        fail_retries(0),
-        connstart(0)
-{}
-
-ConnectStateData::~ConnectStateData()
-{
-    safe_free(host);
-    paths = NULL; // caller code owns them.
-    solo = NULL;
-}
-
-void
-ConnectStateData::callCallback(comm_err_t status, int xerrno)
-{
-    int fd = -1;
-    if (paths != NULL && paths->size() > 0) {
-        fd = (*paths)[0]->fd;
-        debugs(5, 3, HERE << "FD " << fd);
-        comm_remove_close_handler(fd, ConnectStateData::EarlyAbort, this);
-        commSetTimeout(fd, -1, NULL, NULL);
-    }
-
-    typedef CommConnectCbParams Params;
-    Params &params = GetCommParams<Params>(callback);
-    if (solo != NULL) {
-        params.conn = solo;
-    } else if (paths != NULL) {
-        params.paths = paths;
-        if (paths->size() > 0)
-            params.conn = (*paths)[0];
-    } else {
-        /* catch the error case. */
-        assert(paths != NULL && solo != NULL);
-    }
-    params.flag = status;
-    params.xerrno = xerrno;
-    ScheduleCallHere(callback);
-
-    callback = NULL;
-    safe_free(host);
-    delete this;
-}
-
-void
-ConnectStateData::connect()
-{
-    Comm::ConnectionPointer active;
-
-    /* handle connecting to one single path */
-    /* mainly used by components other than forwarding */
-
-    /* handle connecting to one of multiple paths */
-    /* mainly used by forwarding */
-
-    if (solo != NULL) {
-        active = solo;
-    } else if (paths) {
-        Comm::Paths::iterator i = paths->begin();
-
-        if (connstart == 0) {
-            connstart = squid_curtime;
-        }
-
-        /* find some socket we can use. will also bind the local address to it if needed. */
-        while(paths->size() > 0 && (*i)->fd <= 0) {
-#if USE_IPV6
-            /* outbound sockets have no need to be protocol agnostic. */
-            if ((*i)->local.IsIPv6() && (*i)->local.IsIPv4()) {
-                (*i)->local.SetIPv4();
-            }
-#endif
-            (*i)->fd = comm_openex(SOCK_STREAM, IPPROTO_TCP, (*i)->local, (*i)->flags, (*i)->tos, host);
-            if ((*i)->fd <= 0) {
-                debugs(5 , 2, HERE << "Unable to connect " << (*i)->local << " -> " << (*i)->remote << " for " << host);
-                paths->shift();
-                i = paths->begin();
-            }
-            // else success will terminate the loop with: i->fd >0
-        }
-
-        /* we have nowhere left to try connecting */
-        if (paths->size() < 1) {
-            callCallback(COMM_ERR_CONNECT, 0);
-            return;
-        }
-
-        active = (*i);
-    }
-
-    total_tries++;
-
-    switch (comm_connect_addr(active->fd, active->remote) ) {
-
-    case COMM_INPROGRESS:
-        debugs(5, 5, HERE << "FD " << active->fd << ": COMM_INPROGRESS");
-        commSetSelect(active->fd, COMM_SELECT_WRITE, ConnectStateData::ConnectRetry, this, 0);
-        break;
-
-    case COMM_OK:
-        debugs(5, 5, HERE << "FD " << active->fd << ": COMM_OK - connected");
-
-        /*
-         * stats.conn_open is used to account for the number of
-         * connections that we have open to the peer, so we can limit
-         * based on the max-conn option.  We need to increment here,
-         * even if the connection may fail.
-         */
-        if (active->getPeer())
-            active->getPeer()->stats.conn_open++;
-
-        /* TODO: remove these fd_table accesses. But old code still depends on fd_table flags to
-         *       indicate the state of a raw fd object being passed around.
-         */
-        fd_table[active->fd].flags.open = 1;
-        active->local.SetPort(comm_local_port(active->fd));
-
-        ipcacheMarkGoodAddr(host, active->remote);
-        callCallback(COMM_OK, 0);
-        break;
-
-    default:
-        debugs(5, 5, HERE "FD " << active->fd << ": * - try again");
-        fail_retries++;
-        ipcacheMarkBadAddr(host, active->remote);
-
-#if USE_ICMP
-        if (Config.onoff.test_reachability)
-            netdbDeleteAddrNetwork(active->remote);
-#endif
-
-        // check for timeout FIRST.
-        if(squid_curtime - connstart > connect_timeout) {
-            debugs(5, 5, HERE << "FD " << active->fd << ": * - ERR took too long already.");
-            callCallback(COMM_TIMEOUT, errno);
-        } else if (fail_retries < Config.connect_retries) {
-            // check if connect_retries extends the single IP re-try limit.
-            eventAdd("ConnectStateData::Connect", ConnectStateData::Connect, this, 0.5, 0);
-        } else if (paths && paths->size() > 0) {
-            // check if we have more maybe-useful paths to try.
-            paths->shift();
-            fail_retries = 0;
-            eventAdd("ConnectStateData::Connect", ConnectStateData::Connect, this, 0.0, 0);
-        } else {
-            // send ERROR back to the upper layer.
-            debugs(5, 5, HERE << "FD " << active->fd << ": * - ERR tried too many times already.");
-            callCallback(COMM_ERR_CONNECT, errno);
-        }
-    }
-}
-
-void
-ConnectStateData::EarlyAbort(int fd, void *data)
-{
-    ConnectStateData *cs = static_cast<ConnectStateData *>(data);
-    debugs(5, 3, HERE << "FD " << fd);
-    cs->callCallback(COMM_ERR_CLOSING, errno); // NP: is closing or shutdown better?
-
-    /* TODO split cases:
-     * remote end rejecting the connection is normal and one of the other paths may be taken.
-     * squid shutting down or forcing abort on the connection attempt(s) are the only real fatal cases.
-     */
-}
-
-void
-ConnectStateData::Connect(void *data)
-{
-    ConnectStateData *cs = static_cast<ConnectStateData *>(data);
-    cs->connect();
-}
-
-void
-ConnectStateData::ConnectRetry(int fd, void *data)
-{
-    ConnectStateData *cs = static_cast<ConnectStateData *>(data);
-    cs->connect();
-}
diff --git a/src/comm/Connection.cc b/src/comm/Connection.cc
index 3d6e4ec485..1a64c0d6a9 100644
--- a/src/comm/Connection.cc
+++ b/src/comm/Connection.cc
@@ -13,34 +13,32 @@ Comm::Connection::Connection() :
         _peer(NULL)
 {}
 
-Comm::Connection::Connection(const Comm::Connection &c) :
-        local(c.local),
-        remote(c.remote),
-        peer_type(c.peer_type),
-        fd(c.fd),
-        tos(c.tos),
-        flags(c.flags)
+Comm::Connection::~Connection()
 {
-    _peer = cbdataReference(c._peer);
+    close();
+    if (_peer) {
+        cbdataReferenceDone(_peer);
+    }
 }
 
-const Comm::Connection &
-Comm::Connection::operator =(const Comm::Connection &c)
+Comm::ConnectionPointer &
+Comm::Connection::copyDetails() const
 {
-    memcpy(this, &c, sizeof(Comm::Connection));
+    ConnectionPointer c = new Comm::Connection;
 
-    /* ensure we have a cbdata reference to _peer not a straight ptr copy. */
-    _peer = cbdataReference(c._peer);
+    c->local = local;
+    c->remote = remote;
+    c->peer_type = peer_type;
+    c->tos = tos;
+    c->flags = flags;
+ 
+    // ensure FD is not open in the new copy.
+    c->fd = -1;
 
-    return *this;
-}
+    // ensure we have a cbdata reference to _peer not a straight ptr copy.
+    c->_peer = cbdataReference(_peer);
 
-Comm::Connection::~Connection()
-{
-    close();
-    if (_peer) {
-        cbdataReferenceDone(_peer);
-    }
+    return c;
 }
 
 void
diff --git a/src/comm/Connection.h b/src/comm/Connection.h
index 5b35f79289..3f79d1d1c5 100644
--- a/src/comm/Connection.h
+++ b/src/comm/Connection.h
@@ -77,17 +77,21 @@ public:
     /** standard empty connection creation */
     Connection();
 
-    /** Clear the conection properties and close any open socket. */
+    /** These objects may not be exactly duplicated. Use copyDetails() instead. */
+    Connection(const Connection &c);
+
+    /** Clear the connection properties and close any open socket. */
     ~Connection();
 
-    /** Clone an existing connections properties.
-     * This includes the FD, if one is open its a good idea to set it to -1 (unopen)
-     * on one after copying to prevent both clones calling comm_close() when destructed.
+    /** Copy an existing connections IP and properties.
+     * This excludes the FD. The new copy will be a closed connection.
      */
-    Connection(const Connection &c);
-    /** see Comm::Connection::Connection */
-    const Connection & operator =(const Connection &c);
+    ConnectionPointer & copyDetails() const;
+
+    /** These objects may not be exactly duplicated. Use clone() instead. */
+    Connection & operator =(const Connection &c);
 
+    /** Close any open socket. */
     void close();
 
     /** determine whether this object describes an active connection or not. */
diff --git a/src/comm/Makefile.am b/src/comm/Makefile.am
index 00be9af925..beb2111218 100644
--- a/src/comm/Makefile.am
+++ b/src/comm/Makefile.am
@@ -12,8 +12,8 @@ libcomm_la_SOURCES= \
 	ListenStateData.cc \
 	ListenStateData.h \
 	\
-	ConnectStateData.cc \
-	ConnectStateData.h \
+	ConnOpener.cc \
+	ConnOpener.h \
 	\
 	Connection.cc \
 	Connection.h \
diff --git a/src/comm/forward.h b/src/comm/forward.h
index eb30b5ea59..5f0c8a7249 100644
--- a/src/comm/forward.h
+++ b/src/comm/forward.h
@@ -11,7 +11,6 @@ class Connection;
 typedef RefCount<Comm::Connection> ConnectionPointer;
 
 typedef Vector<Comm::ConnectionPointer> Paths;
-typedef Vector<Comm::ConnectionPointer>* PathsPointer;
 
 }; // namespace Comm
 
diff --git a/src/dns_internal.cc b/src/dns_internal.cc
index f495cd99f8..b69d53a299 100644
--- a/src/dns_internal.cc
+++ b/src/dns_internal.cc
@@ -35,7 +35,7 @@
 #include "squid.h"
 #include "CacheManager.h"
 #include "comm/Connection.h"
-#include "comm/ConnectStateData.h"
+#include "comm/ConnOpener.h"
 #include "comm.h"
 #include "event.h"
 #include "fde.h"
@@ -699,7 +699,7 @@ idnsDoSendQueryVC(nsvc *vc)
 }
 
 static void
-idnsInitVCConnected(Comm::ConnectionPointer conn, Comm::PathsPointer unused, comm_err_t status, int xerrno, void *data)
+idnsInitVCConnected(Comm::ConnectionPointer &conn, comm_err_t status, int xerrno, void *data)
 {
     nsvc * vc = (nsvc *)data;
 
@@ -749,9 +749,9 @@ idnsInitVC(int ns)
 
     AsyncCall::Pointer call = commCbCall(78,3, "idnsInitVCConnected", CommConnectCbPtrFun(idnsInitVCConnected, vc));
 
-    ConnectStateData *cs = new ConnectStateData(conn, call);
-    cs->host = xstrdup("DNS TCP Socket");
-    cs->connect();
+    ConnOpener *cs = new ConnOpener(conn, call);
+    cs->setHost("DNS TCP Socket");
+    cs->start();
 }
 
 static void
diff --git a/src/forward.cc b/src/forward.cc
index 4070724700..96727e50b3 100644
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -36,7 +36,7 @@
 #include "acl/Gadgets.h"
 #include "CacheManager.h"
 #include "comm/Connection.h"
-#include "comm/ConnectStateData.h"
+#include "comm/ConnOpener.h"
 #include "CommCalls.h"
 #include "event.h"
 #include "errorpage.h"
@@ -331,10 +331,10 @@ FwdState::complete()
          */
 
         AsyncCall::Pointer call = commCbCall(17,3, "fwdConnectDoneWrapper", CommConnectCbPtrFun(fwdConnectDoneWrapper, this));
-        ConnectStateData *cs = new ConnectStateData(&paths, call);
-        cs->host = xstrdup(entry->url());
+        ConnOpener *cs = new ConnOpener(paths[0], call);
+        cs->setHost(entry->url());
         cs->connect_timeout = Config.Timeout.connect;
-        cs->connect();
+        cs->start();
     } else {
         debugs(17, 3, HERE << "server FD " << paths[0]->fd << " not re-forwarding status " << entry->getReply()->sline.status);
         EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
@@ -351,7 +351,7 @@ FwdState::complete()
 /**** CALLBACK WRAPPERS ************************************************************/
 
 static void
-fwdStartCompleteWrapper(Comm::PathsPointer unused, void *data)
+fwdStartCompleteWrapper(Comm::Paths * unused, void *data)
 {
     FwdState *fwd = (FwdState *) data;
     fwd->startComplete();
@@ -381,10 +381,10 @@ fwdNegotiateSSLWrapper(int fd, void *data)
 #endif
 
 void
-fwdConnectDoneWrapper(Comm::ConnectionPointer conn, Comm::PathsPointer paths, comm_err_t status, int xerrno, void *data)
+fwdConnectDoneWrapper(Comm::ConnectionPointer &conn, comm_err_t status, int xerrno, void *data)
 {
     FwdState *fwd = (FwdState *) data;
-    fwd->connectDone(conn, paths, status, xerrno);
+    fwd->connectDone(conn, status, xerrno);
 }
 
 /**** PRIVATE *****************************************************************/
@@ -504,14 +504,7 @@ FwdState::retryOrBail()
              * A new one will be created if there's another problem */
             err = NULL;
 
-            AsyncCall::Pointer call = commCbCall(17,3,"fwdConnectDoneWrapper", CommConnectCbPtrFun(fwdConnectDoneWrapper, this));
-            ConnectStateData *cs = new ConnectStateData(&paths, call);
-            cs->host = xstrdup(entry->url());
-            cs->connect_timeout = Config.Timeout.connect;
-            cs->connect();
-
-            /* use eventAdd to break potential call sequence loops and to slow things down a little */
-            eventAdd("fwdConnectStart", fwdConnectStartWrapper, this, (paths[0]->getPeer() == NULL) ? 0.05 : 0.005, 0);
+            connectStart();
             return;
         }
         // else bail. no more paths possible to try.
@@ -654,14 +647,11 @@ FwdState::initiateSSL()
 #endif
 
 void
-FwdState::connectDone(Comm::ConnectionPointer conn, Comm::PathsPointer result_paths, comm_err_t status, int xerrno)
+FwdState::connectDone(Comm::ConnectionPointer &conn, comm_err_t status, int xerrno)
 {
-    assert(result_paths == &paths);
-
     if (status != COMM_OK) {
         ErrorState *anErr = errorCon(ERR_CONNECT_FAIL, HTTP_SERVICE_UNAVAILABLE, request);
         anErr->xerrno = xerrno;
-
         fail(anErr);
 
         /* it might have been a timeout with a partially open link */
@@ -671,7 +661,7 @@ FwdState::connectDone(Comm::ConnectionPointer conn, Comm::PathsPointer result_pa
 
             paths[0]->close();
         }
-
+        retryOrBail();
         return;
     }
 
@@ -687,9 +677,6 @@ FwdState::connectDone(Comm::ConnectionPointer conn, Comm::PathsPointer result_pa
     if (paths[0]->getPeer())
         peerConnectSucceded(paths[0]->getPeer());
 
-    // TODO: Avoid this if %<lp is not used? F->local_port is often cached.
-    request->hier.peer_local_port = comm_local_port(paths[0]->fd);
-
     updateHierarchyInfo();
 
 #if USE_SSL
@@ -772,7 +759,7 @@ FwdState::connectStart()
             if (pinned_connection->pinnedAuth())
                 request->flags.auth = 1;
             updateHierarchyInfo();
-            FwdState::connectDone(conn, &paths, COMM_OK, 0);
+            FwdState::connectDone(conn, COMM_OK, 0);
             return;
         }
         /* Failure. Fall back on next path */
@@ -814,9 +801,6 @@ FwdState::connectStart()
 
         comm_add_close_handler(conn->fd, fwdServerClosedWrapper, this);
 
-        // TODO: Avoid this if %<lp is not used? F->local_port is often cached.
-        request->hier.peer_local_port = comm_local_port(conn->fd);
-
         dispatch();
         return;
     }
@@ -826,10 +810,10 @@ FwdState::connectStart()
 #endif
 
     AsyncCall::Pointer call = commCbCall(17,3, "fwdConnectDoneWrapper", CommConnectCbPtrFun(fwdConnectDoneWrapper, this));
-    ConnectStateData *cs = new ConnectStateData(&paths, call);
-    cs->host = xstrdup(host);
+    ConnOpener *cs = new ConnOpener(paths[0], call);
+    cs->setHost(host);
     cs->connect_timeout = ctimeout;
-    cs->connect();
+    cs->start();
 }
 
 void
@@ -1163,6 +1147,8 @@ FwdState::updateHierarchyInfo()
             paths[0]->remote.NtoA(nextHop, 256);
     }
 
+    request->hier.peer_local_port = paths[0]->local.GetPort();
+
     assert(nextHop[0]);
     hierarchyNote(&request->hier, paths[0]->peer_type, nextHop);
 }
diff --git a/src/forward.h b/src/forward.h
index 7463efc43d..11178f1d02 100644
--- a/src/forward.h
+++ b/src/forward.h
@@ -28,7 +28,7 @@ public:
     bool reforwardableStatus(http_status s);
     void serverClosed(int fd);
     void connectStart();
-    void connectDone(Comm::ConnectionPointer conn, Comm::PathsPointer paths, comm_err_t status, int xerrno);
+    void connectDone(Comm::ConnectionPointer &conn, comm_err_t status, int xerrno);
     void connectTimeout(int fd);
     void initiateSSL();
     void negotiateSSL(int fd);
diff --git a/src/ftp.cc b/src/ftp.cc
index 9f3406a12c..7a4380df51 100644
--- a/src/ftp.cc
+++ b/src/ftp.cc
@@ -34,7 +34,7 @@
 
 #include "squid.h"
 #include "comm.h"
-#include "comm/ConnectStateData.h"
+#include "comm/ConnOpener.h"
 #include "comm/ListenStateData.h"
 #include "compat/strtoll.h"
 #include "errorpage.h"
@@ -2418,9 +2418,9 @@ ftpReadEPSV(FtpStateData* ftpState)
     conn->fd = fd;
 
     AsyncCall::Pointer call = commCbCall(9,3, "FtpStateData::ftpPasvCallback", CommConnectCbPtrFun(FtpStateData::ftpPasvCallback, ftpState));
-    ConnectStateData *cs = new ConnectStateData(conn, call);
-    cs->host = xstrdup(fd_table[ftpState->ctrl.fd].ipaddr);
-    cs->connect();
+    ConnOpener *cs = new ConnOpener(conn, call);
+    cs->setHost(ftpState->data.host);
+    cs->start();
 }
 
 /** \ingroup ServerProtocolFTPInternal
@@ -2702,18 +2702,17 @@ ftpReadPasv(FtpStateData * ftpState)
     conn->fd = ftpState->data.fd;
 
     AsyncCall::Pointer call = commCbCall(9,3, "FtpStateData::ftpPasvCallback", CommConnectCbPtrFun(FtpStateData::ftpPasvCallback, ftpState));
-    ConnectStateData *cs = new ConnectStateData(conn, call);
-    cs->host = xstrdup(ftpState->data.host);
+    ConnOpener *cs = new ConnOpener(conn, call);
+    cs->setHost(ftpState->data.host);
     cs->connect_timeout = Config.Timeout.connect;
-    cs->connect();
+    cs->start();
 }
 
 void
-FtpStateData::ftpPasvCallback(Comm::ConnectionPointer conn, Comm::PathsPointer unused, comm_err_t status, int xerrno, void *data)
+FtpStateData::ftpPasvCallback(Comm::ConnectionPointer &conn, comm_err_t status, int xerrno, void *data)
 {
     FtpStateData *ftpState = (FtpStateData *)data;
     debugs(9, 3, HERE);
-// TODO: dead?    ftpState->request->recordLookup(dns);
 
     if (status != COMM_OK) {
         debugs(9, 2, HERE << "Failed to connect. Retrying without PASV.");
diff --git a/src/ident/AclIdent.cc b/src/ident/AclIdent.cc
index 2d0b508316..aa9ecc6b96 100644
--- a/src/ident/AclIdent.cc
+++ b/src/ident/AclIdent.cc
@@ -134,7 +134,8 @@ IdentLookup::checkForAsync(ACLChecklist *cl)const
         Comm::Connection cc; // IDENT will clone it's own copy for alterations.
         cc.local = checklist->conn()->me;
         cc.remote = checklist->conn()->peer;
-        Ident::Start(&cc, LookupDone, checklist);
+        Comm::ConnectionPointer ccp = &cc;
+        Ident::Start(ccp, LookupDone, checklist);
     } else {
         debugs(28, DBG_IMPORTANT, "IdentLookup::checkForAsync: Can't start ident lookup. No client connection" );
         checklist->currentAnswer(ACCESS_DENIED);
diff --git a/src/ident/Ident.cc b/src/ident/Ident.cc
index 65c038bbd4..e56476460d 100644
--- a/src/ident/Ident.cc
+++ b/src/ident/Ident.cc
@@ -38,7 +38,7 @@
 
 #include "comm.h"
 #include "comm/Connection.h"
-#include "comm/ConnectStateData.h"
+#include "comm/ConnOpener.h"
 #include "CommCalls.h"
 #include "ident/Config.h"
 #include "ident/Ident.h"
@@ -59,7 +59,7 @@ typedef struct _IdentClient {
 
 typedef struct _IdentStateData {
     hash_link hash;		/* must be first */
-    Comm::Connection conn;
+    Comm::ConnectionPointer conn;
     IdentClient *clients;
     char buf[4096];
 } IdentStateData;
@@ -103,7 +103,7 @@ Ident::Close(int fdnotused, void *data)
 {
     IdentStateData *state = (IdentStateData *)data;
     identCallback(state, NULL);
-    state->conn.close();
+    state->conn->close();
     hash_remove_link(ident_hash, (hash_link *) state);
     xfree(state->hash.key);
     cbdataFree(state);
@@ -113,23 +113,23 @@ void
 Ident::Timeout(int fd, void *data)
 {
     IdentStateData *state = (IdentStateData *)data;
-    debugs(30, 3, HERE << "FD " << fd << ", " << state->conn.remote);
-    state->conn.close();
+    debugs(30, 3, HERE << "FD " << fd << ", " << state->conn->remote);
+    state->conn->close();
 }
 
 void
-Ident::ConnectDone(Comm::ConnectionPointer conn, Comm::PathsPointer unused, comm_err_t status, int xerrno, void *data)
+Ident::ConnectDone(Comm::ConnectionPointer &conn, comm_err_t status, int xerrno, void *data)
 {
     IdentStateData *state = (IdentStateData *)data;
 
     if (status != COMM_OK) {
         if (status == COMM_TIMEOUT) {
-            debugs(30, 3, "IDENT connection timeout to " << state->conn.remote);
+            debugs(30, 3, "IDENT connection timeout to " << state->conn->remote);
         }
         return;
     }
 
-    assert(conn != NULL && conn == &(state->conn));
+    assert(conn != NULL && conn == state->conn);
 
     /*
      * see if any of our clients still care
@@ -166,10 +166,10 @@ Ident::ReadReply(int fd, char *buf, size_t len, comm_err_t flag, int xerrno, voi
     char *t = NULL;
 
     assert(buf == state->buf);
-    assert(fd == state->conn.fd);
+    assert(fd == state->conn->fd);
 
     if (flag != COMM_OK || len <= 0) {
-        state->conn.close();
+        state->conn->close();
         return;
     }
 
@@ -195,7 +195,7 @@ Ident::ReadReply(int fd, char *buf, size_t len, comm_err_t flag, int xerrno, voi
         }
     }
 
-    state->conn.close();
+    state->conn->close();
 }
 
 void
@@ -218,7 +218,7 @@ CBDATA_TYPE(IdentStateData);
  * start a TCP connection to the peer host on port 113
  */
 void
-Ident::Start(Comm::ConnectionPointer conn, IDCB * callback, void *data)
+Ident::Start(Comm::ConnectionPointer &conn, IDCB * callback, void *data)
 {
     IdentStateData *state;
     char key1[IDENT_KEY_SZ];
@@ -240,19 +240,19 @@ Ident::Start(Comm::ConnectionPointer conn, IDCB * callback, void *data)
     CBDATA_INIT_TYPE(IdentStateData);
     state = cbdataAlloc(IdentStateData);
     state->hash.key = xstrdup(key);
-    /* clone the conn. we are about to destroy the conn
-     * for re-use of the addresses etc by IDENT. */
-    state->conn = *conn;
-    state->conn.local.SetPort(0); // NP: use random port for secure outbound to IDENT_PORT
-    state->conn.flags |= COMM_NONBLOCKING;
+
+    // copy the conn details. We dont want the original FD to be re-used by IDENT.
+    state->conn = conn->copyDetails();
+    // NP: use random port for secure outbound to IDENT_PORT
+    state->conn->local.SetPort(0);
 
     ClientAdd(state, callback, data);
     hash_join(ident_hash, &state->hash);
 
     AsyncCall::Pointer call = commCbCall(30,3, "Ident::ConnectDone", CommConnectCbPtrFun(Ident::ConnectDone, state));
-    ConnectStateData *cs = new ConnectStateData(&(state->conn), call);
+    ConnOpener *cs = new ConnOpener(state->conn, call);
     cs->connect_timeout = Ident::TheConfig.timeout;
-    cs->connect();
+    cs->start();
 }
 
 void
diff --git a/src/ident/Ident.h b/src/ident/Ident.h
index 623018f415..15ed12842a 100644
--- a/src/ident/Ident.h
+++ b/src/ident/Ident.h
@@ -27,7 +27,7 @@ namespace Ident
  * Self-registers with a global ident lookup manager,
  * will call Ident::Init() itself if the manager has not been initialized already.
  */
-void Start(Comm::ConnectionPointer conn, IDCB * callback, void *cbdata);
+void Start(Comm::ConnectionPointer &conn, IDCB * callback, void *cbdata);
 
 /**
  \ingroup IdentAPI
diff --git a/src/neighbors.cc b/src/neighbors.cc
index fd3402b152..81859e30f1 100644
--- a/src/neighbors.cc
+++ b/src/neighbors.cc
@@ -34,7 +34,7 @@
 #include "ProtoPort.h"
 #include "acl/FilledChecklist.h"
 #include "comm/Connection.h"
-#include "comm/ConnectStateData.h"
+#include "comm/ConnOpener.h"
 #include "CacheManager.h"
 #include "event.h"
 #include "htcp.h"
@@ -1353,36 +1353,35 @@ peerProbeConnect(peer * p)
     time_t ctimeout = p->connect_timeout > 0 ? p->connect_timeout : Config.Timeout.peer_connect;
     bool ret = (squid_curtime - p->stats.last_connect_failure) > (ctimeout * 10);
 
-    if (p->testing_now)
+    if (p->testing_now > 0)
         return ret;/* probe already running */
 
     if (squid_curtime - p->stats.last_connect_probe == 0)
         return ret;/* don't probe to often */
 
     /* for each IP address of this peer. find one that we can connect to and probe it. */
-    Comm::PathsPointer paths = new Comm::Paths;
     for (int i = 0; i < p->n_addresses; i++) {
         Comm::ConnectionPointer conn = new Comm::Connection;
         conn->remote = p->addresses[i];
         conn->remote.SetPort(p->http_port);
         getOutgoingAddress(NULL, conn);
-        paths->push_back(conn);
+
+        p->testing_now++;
+
+        AsyncCall::Pointer call = commCbCall(15,3, "peerProbeConnectDone", CommConnectCbPtrFun(peerProbeConnectDone, p));
+        ConnOpener *cs = new ConnOpener(conn, call);
+        cs->connect_timeout = ctimeout;
+        cs->setHost(p->host);
+        cs->start();
     }
 
-    p->testing_now = true;
     p->stats.last_connect_probe = squid_curtime;
 
-    AsyncCall::Pointer call = commCbCall(15,3, "peerProbeConnectDone", CommConnectCbPtrFun(peerProbeConnectDone, p));
-    ConnectStateData *cs = new ConnectStateData(paths, call);
-    cs->connect_timeout = ctimeout;
-    cs->host = xstrdup(p->host);
-    cs->connect();
-
     return ret;
 }
 
 static void
-peerProbeConnectDone(Comm::ConnectionPointer conn, Comm::PathsPointer unused, comm_err_t status, int xerrno, void *data)
+peerProbeConnectDone(Comm::ConnectionPointer &conn, comm_err_t status, int xerrno, void *data)
 {
     peer *p = (peer*)data;
 
@@ -1393,7 +1392,7 @@ peerProbeConnectDone(Comm::ConnectionPointer conn, Comm::PathsPointer unused, co
     }
 
     conn->close();
-    p->testing_now = false;
+    p->testing_now--;
     return;
 }
 
diff --git a/src/peer_select.cc b/src/peer_select.cc
index a61faa8e6b..b38cfebefe 100644
--- a/src/peer_select.cc
+++ b/src/peer_select.cc
@@ -124,7 +124,7 @@ peerSelectIcpPing(HttpRequest * request, int direct, StoreEntry * entry)
 
 
 void
-peerSelect(Comm::PathsPointer paths,
+peerSelect(Comm::Paths * paths,
            HttpRequest * request,
            StoreEntry * entry,
            PSC * callback,
diff --git a/src/structs.h b/src/structs.h
index 95767be1b4..fb85a7e82b 100644
--- a/src/structs.h
+++ b/src/structs.h
@@ -910,7 +910,7 @@ struct peer {
     int n_addresses;
     int rr_count;
     peer *next;
-    bool testing_now;
+    int testing_now;
 
     struct {
         unsigned int hash;
diff --git a/src/tunnel.cc b/src/tunnel.cc
index eb887bc20a..88b7144006 100644
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -37,7 +37,7 @@
 #include "Array.h"
 #include "comm.h"
 #include "comm/Connection.h"
-#include "comm/ConnectStateData.h"
+#include "comm/ConnOpener.h"
 #include "client_side.h"
 #include "client_side_request.h"
 #if DELAY_POOLS
@@ -68,7 +68,7 @@ public:
     char *host;			/* either request->host or proxy host */
     u_short port;
     HttpRequest *request;
-    Comm::PathsPointer paths;
+    Comm::Paths paths;
 
     class Connection
     {
@@ -176,7 +176,7 @@ tunnelStateFree(TunnelStateData * tunnelState)
     assert(tunnelState != NULL);
     assert(tunnelState->noConnections());
     safe_free(tunnelState->url);
-    if (tunnelState->paths) tunnelState->paths->clean();
+    tunnelState->paths.clean();
     tunnelState->host = NULL;
     HTTPMSGUNLOCK(tunnelState->request);
     delete tunnelState;
@@ -466,44 +466,6 @@ TunnelStateData::copyRead(Connection &from, IOCB *completion)
     comm_read(from.fd(), from.buf, from.bytesWanted(1, SQUID_TCP_SO_RCVBUF), completion, this);
 }
 
-#if UNUSED //?
-static void
-tunnelConnectTimeout(int fd, void *data)
-{
-    TunnelStateData *tunnelState = (TunnelStateData *)data;
-    HttpRequest *request = tunnelState->request;
-    ErrorState *err = NULL;
-
-    if (tunnelState->paths != NULL && tunnelState->paths->size() > 0) {
-        if ((*(tunnelState->paths))[0]->getPeer())
-            hierarchyNote(&tunnelState->request->hier, (*(tunnelState->paths))[0]->peer_type,
-                          (*(tunnelState->paths))[0]->getPeer()->host);
-        else if (Config.onoff.log_ip_on_direct)
-            hierarchyNote(&tunnelState->request->hier, (*(tunnelState->paths))[0]->peer_type,
-                          fd_table[tunnelState->server.fd()].ipaddr);
-        else
-            hierarchyNote(&tunnelState->request->hier, (*(tunnelState->paths))[0]->peer_type,
-                          tunnelState->host);
-    } else
-        debugs(26, DBG_IMPORTANT, "tunnelConnectTimeout(): no forwarding destinations available.");
-
-    err = errorCon(ERR_CONNECT_FAIL, HTTP_SERVICE_UNAVAILABLE, request);
-
-    *tunnelState->status_ptr = HTTP_SERVICE_UNAVAILABLE;
-
-    err->xerrno = ETIMEDOUT;
-
-    err->port = tunnelState->port;
-
-    err->callback = tunnelErrorComplete;
-
-    err->callback_data = tunnelState;
-
-    errorSend(tunnelState->client.fd(), err);
-    comm_close(fd);
-}
-#endif
-
 static void
 tunnelConnectedWriteDone(int fd, char *buf, size_t size, comm_err_t flag, int xerrno, void *data)
 {
@@ -558,14 +520,11 @@ tunnelErrorComplete(int fdnotused, void *data, size_t sizenotused)
 
 
 static void
-tunnelConnectDone(Comm::ConnectionPointer unused, Comm::PathsPointer paths, comm_err_t status, int xerrno, void *data)
+tunnelConnectDone(Comm::ConnectionPointer &conn, comm_err_t status, int xerrno, void *data)
 {
     TunnelStateData *tunnelState = (TunnelStateData *)data;
     HttpRequest *request = tunnelState->request;
     ErrorState *err = NULL;
-    Comm::ConnectionPointer conn = (*paths)[0];
-
-    assert(tunnelState->paths == paths);
 
 #if DELAY_POOLS
     /* no point using the delayIsNoDelay stuff since tunnel is nice and simple */
@@ -581,14 +540,27 @@ tunnelConnectDone(Comm::ConnectionPointer unused, Comm::PathsPointer paths, comm
         hierarchyNote(&tunnelState->request->hier, conn->peer_type, tunnelState->host);
 
     if (status != COMM_OK) {
-        err = errorCon(ERR_CONNECT_FAIL, HTTP_SERVICE_UNAVAILABLE, request);
-        *tunnelState->status_ptr = HTTP_SERVICE_UNAVAILABLE;
-        err->xerrno = xerrno;
-        // on timeout is this still:    err->xerrno = ETIMEDOUT;
-        err->port = conn->remote.GetPort();
-        err->callback = tunnelErrorComplete;
-        err->callback_data = tunnelState;
-        errorSend(tunnelState->client.fd(), err);
+        /* At this point only the TCP handshake has failed. no data has been passed.
+         * we are allowed to re-try the TCP-level connection to alternate IPs for CONNECT.
+         */
+        tunnelState->paths.shift();
+        if (status != COMM_TIMEOUT && tunnelState->paths.size() > 0) {
+            /* Try another IP of this destination host */
+            AsyncCall::Pointer call = commCbCall(26,3, "tunnelConnectDone", CommConnectCbPtrFun(tunnelConnectDone, tunnelState));
+            ConnOpener *cs = new ConnOpener(tunnelState->paths[0], call);
+            cs->setHost(tunnelState->url);
+            cs->connect_timeout = Config.Timeout.connect;
+            cs->start();
+        } else {
+            err = errorCon(ERR_CONNECT_FAIL, HTTP_SERVICE_UNAVAILABLE, request);
+            *tunnelState->status_ptr = HTTP_SERVICE_UNAVAILABLE;
+            err->xerrno = xerrno;
+            // on timeout is this still:    err->xerrno = ETIMEDOUT;
+            err->port = conn->remote.GetPort();
+            err->callback = tunnelErrorComplete;
+            err->callback_data = tunnelState;
+            errorSend(tunnelState->client.fd(), err);
+        }
         return;
     }
 
@@ -672,7 +644,7 @@ tunnelStart(ClientHttpRequest * http, int64_t * size_ptr, int *status_ptr)
                    tunnelTimeout,
                    tunnelState);
 
-    peerSelect(tunnelState->paths, request,
+    peerSelect(&(tunnelState->paths), request,
                NULL,
                tunnelPeerSelectComplete,
                tunnelState);
@@ -713,7 +685,7 @@ tunnelProxyConnected(int fd, void *data)
 }
 
 static void
-tunnelPeerSelectComplete(Comm::PathsPointer peer_paths, void *data)
+tunnelPeerSelectComplete(Comm::Paths *peer_paths, void *data)
 {
     TunnelStateData *tunnelState = (TunnelStateData *)data;
     HttpRequest *request = tunnelState->request;
@@ -729,10 +701,10 @@ tunnelPeerSelectComplete(Comm::PathsPointer peer_paths, void *data)
     }
 
     AsyncCall::Pointer call = commCbCall(26,3, "tunnelConnectDone", CommConnectCbPtrFun(tunnelConnectDone, tunnelState));
-    ConnectStateData *cs = new ConnectStateData(tunnelState->paths, call);
-    cs->host = xstrdup(tunnelState->url);
+    ConnOpener *cs = new ConnOpener(tunnelState->paths[0], call);
+    cs->setHost(tunnelState->url);
     cs->connect_timeout = Config.Timeout.connect;
-    cs->connect();
+    cs->start();
 }
 
 CBDATA_CLASS_INIT(TunnelStateData);