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".

diff --git a/src/client_side.cc b/src/client_side.cc
index ba03e748f09083a591b8f3a63d0f79408239ae9e..7c52c87cbcfbe0e09b2bfe13e9b9d56e8b36232b 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -237,16 +237,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);
 }
 
 
@@ -2739,7 +2737,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);
@@ -3662,7 +3661,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;
@@ -3684,14 +3683,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 32aafc9f67e6fa1d61720b598a9ebc4712c1a860..c3b363d5f9d3c30b766052f29d5a8611f0e08394 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -213,7 +213,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);
@@ -279,10 +279,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 a32a71ebbc1b5b46b928125c055695d925004e09..bdbc7b2903fdbdbc2562df9c02f58df19cc72f50 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1120,6 +1120,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 a602b92a5ef9e454de707078e2edcdc3e2401f34..1812b015fff2c88e880677133363e4568fc89e80 100644 (file)
--- 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