From 2818d2cea2a0d839ca0c394ea7e603ea92e45de2 Mon Sep 17 00:00:00 2001
From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Sun, 1 Aug 2021 21:46:49 -0400
Subject: [PATCH] Fixed ConnOpener callback's syncWithComm()

The stale CommConnectCbParams override was testing unused (i.e. always
negative) CommConnectCbParams::fd and was trying to cancel the callback
that most (possibly all) recipients rely on: ConnOpener users expect a
negative answer rather than no answer at all.
---
 src/CommCalls.cc          | 26 ++++++++++++++++++++------
 src/comm/ConnOpener.cc    |  3 ---
 src/tests/stub_libcomm.cc |  1 +
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/CommCalls.cc b/src/CommCalls.cc
index 3becd0aead..c18bc4c6d6 100644
--- a/src/CommCalls.cc
+++ b/src/CommCalls.cc
@@ -53,6 +53,9 @@ CommAcceptCbParams::CommAcceptCbParams(void *aData):
 {
 }
 
+// XXX: Add CommAcceptCbParams::syncWithComm(). Adjust syncWithComm() API if all
+// implementations always return true.
+
 void
 CommAcceptCbParams::print(std::ostream &os) const
 {
@@ -72,13 +75,24 @@ CommConnectCbParams::CommConnectCbParams(void *aData):
 bool
 CommConnectCbParams::syncWithComm()
 {
-    // drop the call if the call was scheduled before comm_close but
-    // is being fired after comm_close
-    if (fd >= 0 && fd_table[fd].closing()) {
-        debugs(5, 3, HERE << "dropping late connect call: FD " << fd);
-        return false;
+    assert(conn);
+
+    // change parameters if this is a successful callback that was scheduled
+    // after its Comm-registered connection started to close
+
+    if (flag != Comm::OK) {
+        assert(!conn->isOpen());
+        return true; // not a successful callback; cannot go out of sync
     }
-    return true; // now we are in sync and can handle the call
+
+    assert(conn->isOpen());
+    if (!fd_table[conn->fd].closing())
+        return true; // no closing in progress; in sync (for now)
+
+    debugs(5, 3, "converting to Comm::ERR_CLOSING: " << conn);
+    conn->noteClosure();
+    flag = Comm::ERR_CLOSING;
+    return true; // now the callback is in sync with Comm again
 }
 
 /* CommIoCbParams */
diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc
index 5625f16065..6c7032b6eb 100644
--- a/src/comm/ConnOpener.cc
+++ b/src/comm/ConnOpener.cc
@@ -143,9 +143,6 @@ Comm::ConnOpener::sendAnswer(Comm::Flag errFlag, int xerrno, const char *why)
             else
                 assert(conn_->isOpen());
 
-            // XXX: CommConnectCbParams imply syncWithComm(). Our recipients
-            // need a callback even if conn is closing. TODO: Do not reuse
-            // CommConnectCbParams; implement a different syncing logic.
             typedef CommConnectCbParams Params;
             Params &params = GetCommParams<Params>(callback_);
             params.conn = conn_;
diff --git a/src/tests/stub_libcomm.cc b/src/tests/stub_libcomm.cc
index 022a7e8242..3d4d32743e 100644
--- a/src/tests/stub_libcomm.cc
+++ b/src/tests/stub_libcomm.cc
@@ -25,6 +25,7 @@ Comm::Connection::~Connection() STUB
 Comm::ConnectionPointer Comm::Connection::cloneIdentDetails() const STUB_RETVAL(nullptr)
 Comm::ConnectionPointer Comm::Connection::cloneDestinationDetails() const STUB_RETVAL(nullptr)
 void Comm::Connection::close() STUB
+void Comm::Connection::noteClosure() STUB
 CachePeer * Comm::Connection::getPeer() const STUB_RETVAL(NULL)
 void Comm::Connection::setPeer(CachePeer * p) STUB
 ScopedId Comm::Connection::codeContextGist() const STUB_RETVAL(id.detach())
-- 
2.47.3