Author: Alex Rousskov <rousskov@measurement-factory.com>

Bug 2505: assertion failed: comm.cc:1727: "p == call"

Do not assert that the close handler being removed must be in
the list because comm_close removes all close handlers before any FD handlers
are fired.

There also seems to be an unrelated(?) problem: comm_remove_close_handler
does not really remove the callback. It only cancels the call. It should
probably remove the callback as well to prevent an unlikely situation where
the close handler list grows "too much".

diff --git a/src/comm.cc b/src/comm.cc
index e4a079243f178965e89459f292c55d197c0370f3..1c965de003c7a2b7da1e22a06ba0200da70e5b59 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -1690,8 +1690,11 @@ comm_remove_close_handler(int fd, PF * handler, void *data)
         if (call->dialer.handler == handler && params.data == data)
             break;             /* This is our handler */
     }
-    assert(p != NULL);
-    p->cancel("comm_remove_close_handler");
+
+    // comm_close removes all close handlers so our handler may be gone
+    if (p != NULL)
+        p->cancel("comm_remove_close_handler");
+    // TODO: should we remove the handler from the close handlers list?
 }
 
 // remove method-based close handler
@@ -1699,14 +1702,17 @@ void
 comm_remove_close_handler(int fd, AsyncCall::Pointer &call)
 {
     assert (isOpen(fd));
-    /* Find handler in list */
     debugs(5, 5, "comm_remove_close_handler: FD " << fd << ", AsyncCall=" << call);
 
+    // comm_close removes all close handlers so our handler may be gone
+    // TODO: should we remove the handler from the close handlers list?
+#if 0
     // Check to see if really exist  the given AsyncCall in comm_close handlers
     // TODO: optimize: this slow code is only needed for the assert() below
     AsyncCall::Pointer p;
     for (p = fd_table[fd].closeHandler; p != NULL && p != call; p = p->Next());
     assert(p == call);
+#endif
 
     call->cancel("comm_remove_close_handler");
 }