NoNewGlobals for iocb_table (#1998)

During shutdown, iocb_table global was deleted, but the corresponding
cleanup code ignored some IoCallback members, triggering misleading
Ipc::UdsSender memory leak reports from Valgrind.

This change uses NoNewGlobals design to address the above problem, but
this code needs a lot more refactoring to address other old problems
associated with Comm initialization.

src/comm.cc

@@ -1152,9 +1152,6 @@ comm_init(void)
 {
     assert(fd_table);
 
-    // make sure the IO pending callback table exists
-    Comm::CallbackTableInit();
-
     /* XXX account fd_table */
     /* Keep a few file descriptors free so that we don't run out of FD's
      * after accepting a client but before it opens a socket or a file.
@@ -1172,8 +1169,6 @@ comm_exit(void)
 {
     delete TheHalfClosed;
     TheHalfClosed = nullptr;
-
-    Comm::CallbackTableDestruct();
 }
 
 #if USE_DELAY_POOLS

src/comm/IoCallback.cc

@@ -16,29 +16,34 @@
 #include "fde.h"
 #include "globals.h"
 
-Comm::CbEntry *Comm::iocb_table;
+namespace Comm
+{
 
-void
-Comm::CallbackTableInit()
+// XXX: Add API to react to Squid_MaxFD changes.
+/// Creates a new callback table using the current value of Squid_MaxFD.
+/// \sa fde::Init()
+static CbEntry *
+MakeCallbackTable()
 {
     // XXX: convert this to a std::map<> ?
-    iocb_table = static_cast<CbEntry*>(xcalloc(Squid_MaxFD, sizeof(CbEntry)));
+    // XXX: Stop bypassing CbEntry-associated constructors! Refactor to use new() instead.
+    const auto iocb_table = static_cast<CbEntry*>(xcalloc(Squid_MaxFD, sizeof(CbEntry)));
     for (int pos = 0; pos < Squid_MaxFD; ++pos) {
         iocb_table[pos].fd = pos;
         iocb_table[pos].readcb.type = IOCB_READ;
         iocb_table[pos].writecb.type = IOCB_WRITE;
     }
+    return iocb_table;
 }
 
-void
-Comm::CallbackTableDestruct()
+} // namespace Comm
+
+Comm::CbEntry &
+Comm::ioCallbacks(const int fd)
 {
-    // release any Comm::Connections being held.
-    for (int pos = 0; pos < Squid_MaxFD; ++pos) {
-        iocb_table[pos].readcb.conn = nullptr;
-        iocb_table[pos].writecb.conn = nullptr;
-    }
-    safe_free(iocb_table);
+    static const auto table = MakeCallbackTable();
+    assert(fd < Squid_MaxFD);
+    return table[fd];
 }
 
 /**

src/comm/IoCallback.h

@@ -58,7 +58,7 @@ class IoCallback
     void reset();
 };
 
-/// Entry nodes for the IO callback table: iocb_table
+/// Entry nodes for the IO callback table.
 /// Keyed off the FD which the event applies to.
 class CbEntry
 {
@@ -70,13 +70,10 @@ class CbEntry
 
 /// Table of scheduled IO events which have yet to be processed ??
 /// Callbacks which might be scheduled in future are stored in fd_table.
-extern CbEntry *iocb_table;
+CbEntry &ioCallbacks(int fd);
 
-void CallbackTableInit();
-void CallbackTableDestruct();
-
-#define COMMIO_FD_READCB(fd)    (&Comm::iocb_table[(fd)].readcb)
-#define COMMIO_FD_WRITECB(fd)   (&Comm::iocb_table[(fd)].writecb)
+#define COMMIO_FD_READCB(fd) (&(Comm::ioCallbacks(fd).readcb))
+#define COMMIO_FD_WRITECB(fd) (&(Comm::ioCallbacks(fd).writecb))
 
 } // namespace Comm
 

src/comm/Write.cc

@@ -49,7 +49,7 @@ Comm::Write(const Comm::ConnectionPointer &conn, const char *buf, int size, Asyn
 
 /** Write to FD.
  * This function is used by the lowest level of IO loop which only has access to FD numbers.
- * We have to use the comm iocb_table to map FD numbers to waiting data and Comm::Connections.
+ * We have to use the Comm::ioCallbacks() to map FD numbers to waiting data and Comm::Connections.
  * Once the write has been concluded we schedule the waiting call with success/fail results.
  */
 void

src/tests/stub_libcomm.cc

@@ -36,22 +36,19 @@ CBDATA_NAMESPACED_CLASS_INIT(Comm, ConnOpener);
 bool Comm::ConnOpener::doneAll() const STUB_RETVAL(false)
 void Comm::ConnOpener::start() STUB
 void Comm::ConnOpener::swanSong() STUB
-Comm::ConnOpener::ConnOpener(const Comm::ConnectionPointer &, const AsyncCall::Pointer &, time_t) : AsyncJob("STUB Comm::ConnOpener") STUB
-    Comm::ConnOpener::~ConnOpener() STUB
-    void Comm::ConnOpener::setHost(const char *) STUB
-    const char * Comm::ConnOpener::getHost() const STUB_RETVAL(nullptr)
+Comm::ConnOpener::ConnOpener(const Comm::ConnectionPointer &, const AsyncCall::Pointer &, time_t) : AsyncJob("STUB Comm::ConnOpener") {STUB}
+Comm::ConnOpener::~ConnOpener() STUB
+void Comm::ConnOpener::setHost(const char *) STUB
+const char * Comm::ConnOpener::getHost() const STUB_RETVAL(nullptr)
 
 #include "comm/forward.h"
-    bool Comm::IsConnOpen(const Comm::ConnectionPointer &) STUB_RETVAL(false)
+bool Comm::IsConnOpen(const Comm::ConnectionPointer &) STUB_RETVAL(false)
 
 #include "comm/IoCallback.h"
-    void Comm::IoCallback::setCallback(iocb_type, AsyncCall::Pointer &, char *, FREE *, int) STUB
-    void Comm::IoCallback::selectOrQueueWrite() STUB
-    void Comm::IoCallback::cancel(const char *) STUB
-    void Comm::IoCallback::finish(Comm::Flag, int) STUB
-    Comm::CbEntry *Comm::iocb_table = nullptr;
-void Comm::CallbackTableInit() STUB
-void Comm::CallbackTableDestruct() STUB
+void Comm::IoCallback::setCallback(iocb_type, AsyncCall::Pointer &, char *, FREE *, int) STUB
+void Comm::IoCallback::selectOrQueueWrite() STUB
+void Comm::IoCallback::cancel(const char *) STUB
+void Comm::IoCallback::finish(Comm::Flag, int) STUB
 
 #include "comm/Loops.h"
 void Comm::SelectLoopInit(void) STUB