Convert ConnOpener callbacks to Async member-function Calls

* Also hold on to the EarlyAbort and Timeout call objects
   This fixes EarlyAbort crashes by allowing proper cancellation

* Also fix httpsAccept() build error.

diff --git a/src/client_side.cc b/src/client_side.cc
index b01e699ba47926ad3482f26f34a90cc74d426c1d..8e7bfc247196f9bc96a7e693c18e42454dbb8f82 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3291,7 +3291,7 @@ clientNegotiateSSL(int fd, void *data)
 
 /** handle a new HTTPS connection */
 static void
-httpsAccept(int sock, int newfd, Comm::ConnectionPointer details,
+httpsAccept(int sock, int newfd, Comm::ConnectionPointer& details,
             comm_err_t flag, int xerrno, void *data)
 {
     https_port_list *s = (https_port_list *)data;

diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc
index 4a4659ebd558f47ffec4a15aa80d99b5dfddf139..56babe393f02f4f63bd217cdca4538a3ae7cdb82 100644 (file)
--- a/src/comm/ConnOpener.cc
+++ b/src/comm/ConnOpener.cc
@@ -2,7 +2,6 @@
 #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"
@@ -18,12 +17,16 @@ ConnOpener::ConnOpener(Comm::ConnectionPointer &c, AsyncCall::Pointer handler) :
         total_tries(0),
         fail_retries(0),
         connstart(0)
-{}
+{
+    memset(&calls, 0, sizeof(calls));
+}
 
 ConnOpener::~ConnOpener()
 {
     safe_free(host);
     solo = NULL;
+    calls.earlyabort = NULL;
+    calls.timeout = NULL;
 }
 
 bool
@@ -40,6 +43,25 @@ ConnOpener::doneAll() const
     return true;
 }
 
+void
+ConnOpener::swanSong()
+{
+    // recover what we can from the job
+    if (solo != NULL && solo->fd > -1) {
+        callCallback(COMM_ERR_CONNECT, 0);
+    }
+
+    // cancel any event watchers
+    if (calls.earlyabort != NULL) {
+        calls.earlyabort->cancel("ConnOpener::swanSong");
+        calls.earlyabort = NULL;
+    }
+    if (calls.timeout != NULL) {
+        calls.timeout->cancel("ConnOpener::swanSong");
+        calls.timeout = NULL;
+    }
+}
+
 void
 ConnOpener::setHost(const char * new_host)
 {
@@ -63,8 +85,16 @@ ConnOpener::callCallback(comm_err_t status, int xerrno)
 {
     /* remove handlers we don't want to happen anymore */
     if (solo != NULL && solo->fd > 0) {
-        comm_remove_close_handler(solo->fd, ConnOpener::EarlyAbort, this);
-        commSetTimeout(solo->fd, -1, NULL, NULL);
+        if (calls.earlyabort != NULL) {
+            comm_remove_close_handler(solo->fd, calls.earlyabort);
+            calls.earlyabort->cancel("ConnOpener completed.");
+            calls.earlyabort = NULL;
+        }
+        if (calls.timeout != NULL) {
+            commSetTimeout(solo->fd, -1, NULL, NULL);
+            calls.timeout->cancel("ConnOpener completed.");
+            calls.timeout = NULL;
+        }
     }
 
     if (callback != NULL) {
@@ -98,14 +128,20 @@ ConnOpener::start()
             return;
         }
 
-        AsyncCall::Pointer ea_call = commCbCall(5,4, "ConnOpener::EarlyAbort",
-                                                CommCloseCbPtrFun(ConnOpener::EarlyAbort, this));
-        comm_add_close_handler(solo->fd, ea_call);
+        if (calls.earlyabort == NULL) {
+            typedef CommCbMemFunT<ConnOpener, CommConnectCbParams> Dialer;
+            calls.earlyabort = asyncCall(5, 4, "ConnOpener::earlyAbort",
+                                         Dialer(this, &ConnOpener::earlyAbort));
+        }
+        comm_add_close_handler(solo->fd, calls.earlyabort);
 
-        AsyncCall::Pointer timeout_call = commCbCall(5,4, "ConnOpener::ConnectTimeout",
-                                                     CommTimeoutCbPtrFun(ConnOpener::ConnectTimeout, this));
+        if (calls.timeout == NULL) {
+            typedef CommCbMemFunT<ConnOpener, CommTimeoutCbParams> Dialer;
+            calls.timeout = asyncCall(5, 4, "ConnOpener::connect Timeout",
+                                      Dialer(this, &ConnOpener::timeout));
+        }
         debugs(5, 3, HERE << "FD " << solo->fd << " timeout " << connect_timeout);
-        commSetTimeout(solo->fd, connect_timeout, timeout_call);
+        commSetTimeout(solo->fd, connect_timeout, calls.timeout);
 
         if (connstart == 0) {
             connstart = squid_curtime;
@@ -176,29 +212,26 @@ ConnOpener::start()
 }
 
 void
-ConnOpener::EarlyAbort(int fd, void *data)
+ConnOpener::earlyAbort(const CommConnectCbParams &io)
 {
-    ConnOpener *cs = static_cast<ConnOpener *>(data);
-    debugs(5, 3, HERE << "FD " << fd);
-    cs->callCallback(COMM_ERR_CLOSING, errno); // NP: is closing or shutdown better?
+    debugs(5, 3, HERE << "FD " << io.conn->fd);
+    callCallback(COMM_ERR_CLOSING, io.xerrno); // NP: is closing or shutdown better?
 }
 
 void
-ConnOpener::Connect(void *data)
+ConnOpener::connect(const CommConnectCbParams &unused)
 {
-    ConnOpener *cs = static_cast<ConnOpener *>(data);
-    cs->start();
+    start();
 }
 
 void
-ConnOpener::ConnectRetry(int fd, void *data)
+ConnOpener::timeout(const CommTimeoutCbParams &unused)
 {
-    ConnOpener *cs = static_cast<ConnOpener *>(data);
-    cs->start();
+    start();
 }
 
 void
-ConnOpener::ConnectTimeout(int fd, void *data)
+ConnOpener::ConnectRetry(int fd, void *data)
 {
     ConnOpener *cs = static_cast<ConnOpener *>(data);
     cs->start();

diff --git a/src/comm/ConnOpener.h b/src/comm/ConnOpener.h
index b8632be161cf62777555e83d71774f62cabe67e3..ca271b9b84f1aecc50d42687a9e4628d7c6fd399 100644 (file)
--- a/src/comm/ConnOpener.h
+++ b/src/comm/ConnOpener.h
@@ -4,6 +4,7 @@
 #include "base/AsyncCall.h"
 #include "base/AsyncJob.h"
 #include "cbdata.h"
+#include "CommCalls.h"
 #include "comm/comm_err_t.h"
 #include "comm/forward.h"
 
@@ -13,40 +14,44 @@
 class ConnOpener : public AsyncJob
 {
 public:
-    /** attempt to open a connection. */
-    ConnOpener(Comm::ConnectionPointer &, AsyncCall::Pointer handler);
-
-    ~ConnOpener();
+    // ****** AsynJob API implementation ******
 
     /** Actual start opening a TCP connection. */
     void start();
 
     virtual bool doneAll() const;
+    virtual void swanSong();
+
+    // ****** ConnOpener API iplementation ******
+
+    /** attempt to open a connection. */
+    ConnOpener(Comm::ConnectionPointer &, AsyncCall::Pointer handler);
+    ~ConnOpener();
+
+    void setHost(const char *);    ///< set the hostname note for this connection
+    const char * getHost() const;  ///< get the hostname noted for this connection
+
 private:
     /* These objects may NOT be created without connections to act on. Do not define this operator. */
     ConnOpener(const ConnOpener &);
     /* These objects may NOT be copied. Do not define this operator. */
     ConnOpener operator =(const ConnOpener &c);
 
-    /**
-     * Wrapper to start the connection attempts happening.
+    /** Make an FD connection attempt.
+     * Handles the case(s) when a partially setup connection gets closed early.
      */
-    static void Connect(void *data);
-
-    /** retry */
-    static void ConnectRetry(int fd, void *data);
+    void connect(const CommConnectCbParams &unused);
 
-    /**
-     * Temporary close handler used during connect.
+    /** Abort connection attempt.
      * Handles the case(s) when a partially setup connection gets closed early.
      */
-    static void EarlyAbort(int fd, void *data);
+    void earlyAbort(const CommConnectCbParams &);
 
     /**
-     * Temporary timeout handler used during connect.
      * Handles the case(s) when a partially setup connection gets timed out.
+     * NP: When commSetTimeout accepts generic CommCommonCbParams this can die.
      */
-    static void ConnectTimeout(int fd, void *data);
+    void timeout(const CommTimeoutCbParams &unused);
 
     /**
      * Connection attempt are completed. One way or the other.
@@ -54,6 +59,10 @@ private:
      */
     void callCallback(comm_err_t status, int xerrno);
 
+    // Legacy Wrapper for the retry event after COMM_INPROGRESS
+    // As soon as comm IO accepts Async calls we can use a ConnOpener::connect call
+    static void ConnectRetry(int fd, void *data);
+
 public:
     /**
      * time at which to abandon the connection.
@@ -61,9 +70,6 @@ public:
      */
     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:
     char *host;                         ///< domain name we are trying to connect to.
 
@@ -74,6 +80,12 @@ 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.
 
+    /// handles to calls which we may need to cancel.
+    struct _calls {
+        AsyncCall::Pointer earlyabort;
+        AsyncCall::Pointer timeout;
+    } calls;
+
     CBDATA_CLASS2(ConnOpener);
 };