From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Thu, 11 Sep 2008 04:54:34 +0000 (-0600)
Subject: Cleaned up ConnStateData's closing and destruction.
X-Git-Tag: SQUID_3_1_0_1~49^2~47^2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=6e1d409c65c63e90343e9156a43dbbc87ff96e59;p=thirdparty%2Fsquid.git

Cleaned up ConnStateData's closing and destruction.

1) Despite its name and the "if (open) close" use in ConnStateData destructor,
ConnStateData::close() was not closing anything. It was called from the Comm
close handler and from the destructor and would attempt to immediately delete
the ConnStateData object. Protecting code in deleteThis() may have prevented
the actual [double] delete from happening, but it is difficult to say exactly
what was going on when close() was being called from the destructor.

I converted ConnStateData::close to swanSong, which is the standard AsyncJob
cleanup method. As before, the method does not close anything (which may be
wrong). The swanSong method is never called directly by the user code. It is
called by lower layers just before the job is destroyed.

We may need to add Comm closing code to swanSong. For now, the updated
ConnStateData destructor will warn if ConnStateData forgot to close
the connection. The destructor will also warn if swanSong was not called,
which would mean that the job object is being deleted incorrectly.


2) Polished ClientSocketContext::writeComplete to distinguish
STREAM_UNPLANNED_COMPLETE from STREAM_FAILED closing state. This helps when
looking at stack traces.


3) Added an XXX comment about duplicated code.


4) Documented ClientSocketContext::initiateClose purpose and context.
---

diff --git a/src/client_side.cc b/src/client_side.cc
index 043d9026a7..325f3649cc 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -602,14 +602,14 @@ ConnStateData::freeAllContexts()
 void ConnStateData::connStateClosed(const CommCloseCbParams &io)
 {
     assert (fd == io.fd);
-    close();
+    deleteThis("ConnStateData::connStateClosed");
 }
 
+// cleans up before destructor is called
 void
-ConnStateData::close()
+ConnStateData::swanSong()
 {
-    debugs(33, 3, "ConnStateData::close: FD " << fd);
-    deleteThis("ConnStateData::close");
+    debugs(33, 2, "ConnStateData::swanSong: FD " << fd);
     fd = -1;
     flags.readMoreRequests = false;
     clientdbEstablished(peer, -1);	/* decrement */
@@ -617,9 +617,12 @@ ConnStateData::close()
     freeAllContexts();
 
     if (auth_user_request != NULL) {
-        debugs(33, 4, "ConnStateData::close: freeing auth_user_request '" << auth_user_request << "' (this is '" << this << "')");
+        debugs(33, 4, "ConnStateData::swanSong: freeing auth_user_request '" << auth_user_request << "' (this is '" << this << "')");
         auth_user_request->onConnectionClose(this);
     }
+
+    BodyProducer::swanSong();
+    flags.swanSang = true;
 }
 
 bool
@@ -636,7 +639,10 @@ ConnStateData::~ConnStateData()
     debugs(33, 3, "ConnStateData::~ConnStateData: FD " << fd);
 
     if (isOpen())
-        close();
+        debugs(33, 1, "BUG: ConnStateData did not close FD " << fd);
+
+    if (!flags.swanSang)
+        debugs(33, 1, "BUG: ConnStateData was not destroyed properly; FD " << fd);
 
     AUTHUSERREQUESTUNLOCK(auth_user_request, "~conn");
 
@@ -1587,6 +1593,8 @@ ClientSocketContext::doClose()
     comm_close(fd());
 }
 
+/** Called to initiate (and possibly complete) closing of the context.
+ * The underlying socket may be already closed */
 void
 ClientSocketContext::initiateClose(const char *reason)
 {
@@ -1660,10 +1668,11 @@ ClientSocketContext::writeComplete(int fd, char *bufnotused, size_t size, comm_e
         return;
 
     case STREAM_UNPLANNED_COMPLETE:
-        /* fallthrough */
+        initiateClose("STREAM_UNPLANNED_COMPLETE");
+        return;
 
     case STREAM_FAILED:
-        initiateClose("STREAM_UNPLANNED_COMPLETE|STREAM_FAILED");
+        initiateClose("STREAM_FAILED");
         return;
 
     default:
@@ -2551,6 +2560,7 @@ ConnStateData::clientReadRequest(const CommIoCbParams &io)
          * The above check with connFinishedWithConn() only
          * succeeds _if_ the buffer is empty which it won't
          * be if we have an incomplete request.
+         * XXX: This duplicates ClientSocketContext::keepaliveNextRequest
          */
         if (getConcurrentRequestCount() == 0 && commIsHalfClosed(fd)) {
             debugs(33, 5, "clientReadRequest: FD " << fd << ": half-closed connection, no completed request parsed, connection closing.");
diff --git a/src/client_side.h b/src/client_side.h
index 28eb767f2d..def37ed7cf 100644
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -142,7 +142,6 @@ public:
     ClientSocketContext::Pointer getCurrentContext() const;
     void addContextToQueue(ClientSocketContext * context);
     int getConcurrentRequestCount() const;
-    void close();
     bool isOpen() const;
 
     int fd;
@@ -188,6 +187,7 @@ public:
     struct
     {
         bool readMoreRequests;
+        bool swanSang; // XXX: temporary flag to check proper cleanup
     } flags;
     http_port_list *port;
 
@@ -213,6 +213,7 @@ public:
 
     // AsyncJob API
     virtual bool doneAll() const { return BodyProducer::doneAll() && false;}
+    virtual void swanSong();
 
 #if USE_SSL
     bool switchToHttps();