Author: Christos Tsantilas <chtsanti@users.sourceforge.net>

Bug 2253: Assertion in comm closing sequence (pt 1)

diff --git a/src/client_side.cc b/src/client_side.cc
index 7faf19992539a6c52eebba25a2e5d8fd0538c9f5..043d9026a7101fd0a1899d054c57a51379603a51 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -625,7 +625,9 @@ ConnStateData::close()
 bool
 ConnStateData::isOpen() const
 {
-    return cbdataReferenceValid(this);
+    return cbdataReferenceValid(this) && // XXX: checking "this" in a method
+        fd >= 0 &&
+        !fd_table[fd].closing();
 }
 
 ConnStateData::~ConnStateData()
@@ -2301,10 +2303,7 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c
         connNoteUseOfBuffer(conn, http->req_sz);
         notedUseOfBuffer = true;
 
-        conn->handleRequestBodyData();
-
-        if (!request->body_pipe->exhausted())
-            conn->readSomeData();
+        conn->handleRequestBodyData(); // may comm_close and stop producing
 
         /* Is it too large? */
 
@@ -2321,7 +2320,10 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c
             goto finish;
         }
 
-        context->mayUseConnection(true);
+        if (!request->body_pipe->productionEnded())
+            conn->readSomeData();
+
+        context->mayUseConnection(!request->body_pipe->productionEnded());
     }
 
     http->calloutContext = new ClientRequestContext(http);

diff --git a/src/comm.cc b/src/comm.cc
index 565f879ebddbb3a2465b24fd8b49d6ca9f181eaa..ae52476cf7ccb0f261d2d3f959cc783bd6ece619 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -336,7 +336,10 @@ comm_read(int fd, char *buf, int size, AsyncCall::Pointer &callback)
 {
     /* Make sure we're not reading anything and we're not closing */
     assert(isOpen(fd));
-    assert(!fd_table[fd].flags.closing);
+    assert(!fd_table[fd].flags.closing_);
+    // XXX: If we already called commio_finish_callback, the new callback 
+    // we are setting here would apply to the next connection with the same FD.
+    assert(!fd_table[fd].flags.close_request); 
 
     debugs(5, 4, "comm_read, queueing read for FD " << fd);
 
@@ -1489,6 +1492,23 @@ CommRead::doCallback(comm_err_t errcode, int xerrno)
     }
 }
 
+void 
+comm_close_start(int fd, void *data)
+{
+    fde *F = &fd_table[fd];
+
+    F->flags.closing_ = 1;
+
+#if USE_SSL
+
+    if (F->ssl)
+        ssl_shutdown_method(fd);
+
+#endif
+
+}
+
+
 void 
 comm_close_complete(int fd, void *data)
 {
@@ -1541,7 +1561,10 @@ _comm_close(int fd, char const *file, int line)
     fdd_table[fd].close_file = file;
     fdd_table[fd].close_line = line;
 
-    if (F->flags.closing)
+    if(F->flags.close_request)
+       return;
+
+    if (F->flags.closing_)
         return;
 
     if (shutting_down && (!F->flags.open || F->type == FD_FILE))
@@ -1556,14 +1579,14 @@ _comm_close(int fd, char const *file, int line)
 
     PROF_start(comm_close);
 
-    F->flags.closing = 1;
-
-#if USE_SSL
-
-    if (F->ssl)
-        ssl_shutdown_method(fd);
+    F->flags.close_request = 1;
 
-#endif
+    AsyncCall::Pointer startCall=commCbCall(5,4, "comm_close_start",
+                                           CommCloseCbPtrFun(comm_close_start, NULL));
+    typedef CommCloseCbParams Params;
+    Params &startParams = GetCommParams<Params>(startCall);
+    startParams.fd = fd;
+    ScheduleCallHere(startCall);
 
     commSetTimeout(fd, -1, NULL, NULL);
 
@@ -1586,12 +1609,11 @@ _comm_close(int fd, char const *file, int line)
     comm_empty_os_read_buffers(fd);
     
 
-    AsyncCall::Pointer call=commCbCall(5,4, "comm_close_complete",
+    AsyncCall::Pointer completeCall=commCbCall(5,4, "comm_close_complete",
                                       CommCloseCbPtrFun(comm_close_complete, NULL));
-    typedef CommCloseCbParams Params;
-    Params &params = GetCommParams<Params>(call);
-    params.fd = fd;
-    ScheduleCallHere(call);
+    Params &completeParams = GetCommParams<Params>(completeCall);
+    completeParams.fd = fd;
+    ScheduleCallHere(completeCall);
 
     PROF_stop(comm_close);
 }
@@ -1995,7 +2017,7 @@ comm_write(int fd, const char *buf, int size, IOCB * handler, void *handler_data
 void
 comm_write(int fd, const char *buf, int size, AsyncCall::Pointer &callback, FREE * free_func)
 {
-    assert(!fd_table[fd].flags.closing);
+    assert(!fd_table[fd].flags.closing_);
 
     debugs(5, 5, "comm_write: FD " << fd << ": sz " << size << ": asynCall " << callback  << ".");
 
@@ -2167,7 +2189,6 @@ comm_listen(int sock) {
     return sock;
 }
 
-// AcceptFD::callback() wrapper
 void
 comm_accept(int fd, IOACB *handler, void *handler_data) {
     debugs(5, 5, "comm_accept: FD " << fd << " handler: " << (void*)handler);
@@ -2234,7 +2255,7 @@ AcceptFD::acceptOne() {
 
         if (newfd == COMM_NOMESSAGE) {
             /* register interest again */
-            debugs(5, 5, "AcceptFD::acceptOne eof: FD " << fd <<
+            debugs(5, 5, HERE << "try later: FD " << fd <<
                 " handler: " << *theCallback);
             commSetSelect(fd, COMM_SELECT_READ, comm_accept_try, NULL, 0);
             return false;

diff --git a/src/disk.cc b/src/disk.cc
index 69897e1a463561cf122d69503d0406bd27019dbf..b78afe37e9fdd3157398c86f3b7e35c46e38a662 100644 (file)
--- a/src/disk.cc
+++ b/src/disk.cc
@@ -133,7 +133,7 @@ file_close(int fd)
      */
     assert(F->write_handler == NULL);
 
-    F->flags.closing = 1;
+    F->flags.closing_ = 1;
 
 #if CALL_FSYNC_BEFORE_CLOSE
 

diff --git a/src/fde.h b/src/fde.h
index 517c54dfce9a804beda181971c4462eb5e714c75..a4403af0adb2e80b0e8734637f033789cefef669 100644 (file)
--- a/src/fde.h
+++ b/src/fde.h
@@ -49,6 +49,15 @@ public:
         memset(this, 0, sizeof(fde));
         local_addr.SetEmpty(); // IPAddress likes to be setup nicely.
     }
+    
+    /**
+     * Return true if the the comm_close for this fd called.
+     * Two flags used for the filedescriptor closing procedure:
+     * - The flag flags.close_request which set when the comm_close called
+     * - The flag flags.closing which scheduled to be set just before the 
+     *    comm_close handlers called.
+     */
+    bool closing() {return flags.closing_ || flags.close_request;}
 
     /* NOTE: memset is used on fdes today. 20030715 RBC */
     static void DumpStats (StoreEntry *);
@@ -72,7 +81,7 @@ public:
        unsigned int open:1;
        unsigned int close_request:1;
        unsigned int write_daemon:1;
-       unsigned int closing:1;
+       unsigned int closing_:1;
        unsigned int socket_eof:1;
        unsigned int nolinger:1;
        unsigned int nonblocking:1;