From 56536ae0395e3d49defe3acf4271f74e26a4184e Mon Sep 17 00:00:00 2001
From: Amos Jeffries <amosjeffries@squid-cache.org>
Date: Thu, 22 Jul 2010 22:30:08 -0600
Subject: [PATCH] Author: Alex Rousskov <rousskov@measurement-factory.com>
 Fixed comm.cc:377: "fd_table[fd].halfClosedReader != NULL" assertion

Client side must stop reading when switching to a tunnel mode. The old code
called low-level commSetSelect to stop reading, but that left Comm tables in
an inconsistent state, with the client side reader callback still scheduled.
Squid would assert when the tunnel called comm_read with its own callback.

The bug is unrelated to half-closed connections despite halfClosedReader
mentioned in the assertion text. The assertion means "no more than one active
reader per FD".
---
 src/client_side.cc         | 23 ++++++++++++-----------
 src/client_side.h          |  4 ++--
 src/client_side_request.cc |  1 +
 src/tunnel.cc              |  5 -----
 4 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/src/client_side.cc b/src/client_side.cc
index fcf16e86d9..a3f4b8d90d 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -205,16 +205,14 @@ ConnStateData::readSomeData()
     if (reading())
         return;
 
-    reading(true);
-
     debugs(33, 4, "clientReadSomeData: FD " << fd << ": reading request...");
 
     makeSpaceAvailable();
 
     typedef CommCbMemFunT<ConnStateData, CommIoCbParams> Dialer;
-    AsyncCall::Pointer call = asyncCall(33, 5, "ConnStateData::clientReadRequest",
-                                        Dialer(this, &ConnStateData::clientReadRequest));
-    comm_read(fd, in.addressToReadInto(), getAvailableBufferLength(), call);
+    reader = asyncCall(33, 5, "ConnStateData::clientReadRequest",
+                       Dialer(this, &ConnStateData::clientReadRequest));
+    comm_read(fd, in.addressToReadInto(), getAvailableBufferLength(), reader);
 }
 
 
@@ -2669,7 +2667,8 @@ void
 ConnStateData::clientReadRequest(const CommIoCbParams &io)
 {
     debugs(33,5,HERE << "clientReadRequest FD " << io.fd << " size " << io.size);
-    reading(false);
+    Must(reading());
+    reader = NULL;
     bool do_next_read = 1; /* the default _is_ to read data! - adrian */
 
     assert (io.fd == fd);
@@ -3563,7 +3562,7 @@ clientAclChecklistCreate(const acl_access * acl, ClientHttpRequest * http)
 
 CBDATA_CLASS_INIT(ConnStateData);
 
-ConnStateData::ConnStateData() :AsyncJob("ConnStateData"), transparent_ (false), reading_ (false), closing_ (false)
+ConnStateData::ConnStateData() :AsyncJob("ConnStateData"), transparent_ (false), closing_ (false)
 {
     pinning.fd = -1;
     pinning.pinned = false;
@@ -3585,14 +3584,16 @@ ConnStateData::transparent(bool const anInt)
 bool
 ConnStateData::reading() const
 {
-    return reading_;
+    return reader != NULL;
 }
 
 void
-ConnStateData::reading(bool const newBool)
+ConnStateData::stopReading()
 {
-    assert (reading() != newBool);
-    reading_ = newBool;
+    if (reading()) {
+        comm_read_cancel(fd, reader);
+        reader = NULL;
+    }
 }
 
 
diff --git a/src/client_side.h b/src/client_side.h
index 2c8ef911b7..5824b63966 100644
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -212,7 +212,7 @@ public:
     bool transparent() const;
     void transparent(bool const);
     bool reading() const;
-    void reading(bool const);
+    void stopReading(); ///< cancels comm_read if it is scheduled
 
     bool closing() const;
     void startClosing(const char *reason);
@@ -278,10 +278,10 @@ private:
 private:
     CBDATA_CLASS2(ConnStateData);
     bool transparent_;
-    bool reading_;
     bool closing_;
 
     bool switchedToHttps_;
+    AsyncCall::Pointer reader; ///< set when we are reading
     BodyPipe::Pointer bodyPipe; // set when we are reading request body
 };
 
diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 6e8d3a6e89..b6ca7b7ce9 100644
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1114,6 +1114,7 @@ ClientHttpRequest::processRequest()
 
     if (request->method == METHOD_CONNECT && !redirect.status) {
         logType = LOG_TCP_MISS;
+        getConn()->stopReading(); // tunnels read for themselves
         tunnelStart(this, &out.size, &al.http.code);
         return;
     }
diff --git a/src/tunnel.cc b/src/tunnel.cc
index ba33df3c95..8948c5dc39 100644
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -691,11 +691,6 @@ tunnelStart(ClientHttpRequest * http, int64_t * size_ptr, int *status_ptr)
                NULL,
                tunnelPeerSelectComplete,
                tunnelState);
-    /*
-     * Disable the client read handler until peer selection is complete
-     * Take control away from client_side.c.
-     */
-    commSetSelect(tunnelState->client.fd(), COMM_SELECT_READ, NULL, NULL, 0);
 }
 
 static void
-- 
2.47.3