Cleanup: Simplify HTTP 1xx control message writing

The ::Server class heirarchy has the responsibility of writing to
client connection sockets. The logic that was in ClientSocketContext
can be moved (unchanged) to HttpControlMsgSink and ConnStateData.
There was actually never any need to have it spread outside the
ConnStateData class hierarchy in the first place.

No logic is changed in this patch, just symbol shuffling and one
method inlined into its caller.

diff --git a/src/HttpControlMsg.cc b/src/HttpControlMsg.cc
new file mode 100644 (file)
index 0000000..e333701
--- /dev/null
+++ b/src/HttpControlMsg.cc
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#include "squid.h"
+#include "comm/Flag.h"
+#include "CommCalls.h"
+#include "HttpControlMsg.h"
+
+/// called when we wrote the 1xx response
+void
+HttpControlMsgSink::wroteControlMsg(const CommIoCbParams &params)
+{
+    if (params.flag == Comm::ERR_CLOSING)
+        return;
+
+    if (params.flag == Comm::OK) {
+        if (cbControlMsgSent)
+            ScheduleCallHere(cbControlMsgSent);
+        return;
+    }
+
+    debugs(33, 3, "1xx writing failed: " << xstrerr(params.xerrno));
+    // no error notification: see HttpControlMsg.h for rationale and
+    // note that some errors are detected elsewhere (e.g., close handler)
+
+    // close on 1xx errors to be conservative and to simplify the code
+    // (if we do not close, we must notify the source of a failure!)
+    params.conn->close();
+
+    // XXX: writeControlMsgAndCall() should handle writer-specific writing
+    // results, including errors and then call us with success/failure outcome.
+}
+

diff --git a/src/HttpControlMsg.h b/src/HttpControlMsg.h
index f424157f3be49465b2062f899b772bb361903071..bb5c9b137acf4f113ae99f66454bb1b7855218aa 100644 (file)
--- a/src/HttpControlMsg.h
+++ b/src/HttpControlMsg.h
@@ -12,6 +12,7 @@
 #include "base/AsyncCall.h"
 #include "HttpReply.h"
 
+class CommIoCbParams;
 class HttpControlMsg;
 
 /*
@@ -31,6 +32,12 @@ public:
 
     /// called to send the 1xx message and notify the Source
     virtual void sendControlMsg(HttpControlMsg msg) = 0;
+
+    /// callback to handle Comm::Write completion
+    void wroteControlMsg(const CommIoCbParams &);
+
+    /// Call to schedule when the control msg has been sent
+    AsyncCall::Pointer cbControlMsgSent;
 };
 
 /// bundles HTTP 1xx reply and the "successfully forwarded" callback

diff --git a/src/Makefile.am b/src/Makefile.am
index f56a7e7bb3dca4c64dfd07ba9ef79872c58b66b0..7e752d37eb310db5867713c37c3a7835576979ed 100644 (file)
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -347,6 +347,7 @@ squid_SOURCES = \
        HttpHeaderTools.cc \
        HttpBody.h \
        HttpBody.cc \
+       HttpControlMsg.cc \
        HttpControlMsg.h \
        HttpMsg.cc \
        HttpMsg.h \
@@ -981,6 +982,7 @@ tests_testHttpReply_SOURCES=\
        HttpHeaderFieldInfo.h \
        HttpHeaderTools.h \
        HttpHeaderTools.cc \
+       HttpControlMsg.cc \
        HttpControlMsg.h \
        HttpMsg.cc \
        HttpMsg.h \
@@ -1254,6 +1256,7 @@ tests_testCacheManager_SOURCES = \
        tests/testCacheManager.cc \
        tests/testCacheManager.h \
        tests/stub_main_cc.cc \
+       tests/stub_HttpControlMsg.cc \
        tests/stub_ipc_Forwarder.cc \
        tests/stub_store_stats.cc \
        tests/stub_EventLoop.cc \
@@ -1754,6 +1757,7 @@ tests_testEvent_SOURCES = \
        http.cc \
        HttpBody.h \
        HttpBody.cc \
+       tests/stub_HttpControlMsg.cc \
        HttpHeader.h \
        HttpHeader.cc \
        HttpHeaderFieldInfo.h \
@@ -2001,6 +2005,7 @@ tests_testEventLoop_SOURCES = \
        http.cc \
        HttpBody.h \
        HttpBody.cc \
+       tests/stub_HttpControlMsg.cc \
        HttpHeader.h \
        HttpHeader.cc \
        HttpHeaderFieldInfo.h \
@@ -2243,6 +2248,7 @@ tests_test_http_range_SOURCES = \
        http.cc \
        HttpBody.h \
        HttpBody.cc \
+       tests/stub_HttpControlMsg.cc \
        HttpHeaderFieldStat.h \
        HttpHdrCc.h \
        HttpHdrCc.cc \
@@ -2574,6 +2580,7 @@ tests_testHttpRequest_SOURCES = \
        http.cc \
        HttpBody.h \
        HttpBody.cc \
+       tests/stub_HttpControlMsg.cc \
        HttpHeader.h \
        HttpHeader.cc \
        HttpHeaderFieldInfo.h \
@@ -3427,6 +3434,7 @@ tests_testURL_SOURCES = \
        http.cc \
        HttpBody.h \
        HttpBody.cc \
+       tests/stub_HttpControlMsg.cc \
        HttpHeaderFieldStat.h \
        HttpHdrCc.h \
        HttpHdrCc.cc \

diff --git a/src/client_side.cc b/src/client_side.cc
index 350e568802822fc71889cae34273248d7b0ef7bc..02894faf45f7c5569b59df4bf2af15405a0344de 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -279,53 +279,6 @@ ClientSocketContext::ClientSocketContext(const Comm::ConnectionPointer &aConn, C
     deferredparams.rep = NULL;
 }
 
-void
-ClientSocketContext::writeControlMsg(HttpControlMsg &msg)
-{
-    HttpReply::Pointer rep(msg.reply);
-    Must(rep != NULL);
-
-    // remember the callback
-    cbControlMsgSent = msg.cbSuccess;
-
-    AsyncCall::Pointer call = commCbCall(33, 5, "ClientSocketContext::wroteControlMsg",
-                                         CommIoCbPtrFun(&WroteControlMsg, this));
-
-    getConn()->writeControlMsgAndCall(this, rep.getRaw(), call);
-}
-
-/// called when we wrote the 1xx response
-void
-ClientSocketContext::wroteControlMsg(const Comm::ConnectionPointer &conn, char *, size_t, Comm::Flag errflag, int xerrno)
-{
-    if (errflag == Comm::ERR_CLOSING)
-        return;
-
-    if (errflag == Comm::OK) {
-        ScheduleCallHere(cbControlMsgSent);
-        return;
-    }
-
-    debugs(33, 3, HERE << "1xx writing failed: " << xstrerr(xerrno));
-    // no error notification: see HttpControlMsg.h for rationale and
-    // note that some errors are detected elsewhere (e.g., close handler)
-
-    // close on 1xx errors to be conservative and to simplify the code
-    // (if we do not close, we must notify the source of a failure!)
-    conn->close();
-
-    // XXX: writeControlMsgAndCall() should handle writer-specific writing
-    // results, including errors and then call us with success/failure outcome.
-}
-
-/// wroteControlMsg() wrapper: ClientSocketContext is not an AsyncJob
-void
-ClientSocketContext::WroteControlMsg(const Comm::ConnectionPointer &conn, char *bufnotused, size_t size, Comm::Flag errflag, int xerrno, void *data)
-{
-    ClientSocketContext *context = static_cast<ClientSocketContext*>(data);
-    context->wroteControlMsg(conn, bufnotused, size, errflag, xerrno);
-}
-
 #if USE_IDENT
 static void
 clientIdentDone(const char *ident, void *data)
@@ -4506,8 +4459,17 @@ ConnStateData::sendControlMsg(HttpControlMsg msg)
         return;
     }
 
+    // HTTP/1 1xx status messages are only valid when there is a transaction to trigger them
     if (!pipeline.empty()) {
-        pipeline.front()->writeControlMsg(msg); // will call msg.cbSuccess
+        HttpReply::Pointer rep(msg.reply);
+        Must(rep);
+        // remember the callback
+        cbControlMsgSent = msg.cbSuccess;
+
+        typedef CommCbMemFunT<HttpControlMsgSink, CommIoCbParams> Dialer;
+        AsyncCall::Pointer call = JobCallback(33, 5, Dialer, this, HttpControlMsgSink::wroteControlMsg);
+
+        writeControlMsgAndCall(rep.getRaw(), call);
         return;
     }
 

diff --git a/src/client_side.h b/src/client_side.h
index bce603db72bcb418e0d6ed942c159d5b8877de4e..c6f6a040a5ff1e7492973828cd690fea8c1593f0 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -139,13 +139,6 @@ public:
     void registerWithConn();
     void noteIoError(const int xerrno); ///< update state to reflect I/O error
 
-    /// starts writing 1xx control message to the client
-    void writeControlMsg(HttpControlMsg &msg);
-
-protected:
-    static IOCB WroteControlMsg;
-    void wroteControlMsg(const Comm::ConnectionPointer &conn, char *bufnotused, size_t size, Comm::Flag errflag, int xerrno);
-
 private:
     void prepareReply(HttpReply * rep);
     void packChunk(const StoreIOBuffer &bodyData, MemBuf &mb);
@@ -153,8 +146,6 @@ private:
     void doClose();
     void initiateClose(const char *reason);
 
-    AsyncCall::Pointer cbControlMsgSent; ///< notifies HttpControlMsg Source
-
     bool mayUseConnection_; /* This request may use the connection. Don't read anymore requests for now */
     bool connRegistered_;
 };
@@ -203,6 +194,9 @@ public:
     virtual bool handleReadData();
     virtual void afterClientRead();
 
+    /* HttpControlMsgSink API */
+    virtual void sendControlMsg(HttpControlMsg);
+
     /// Traffic parsing
     bool clientParseRequests();
     void readNextRequest();
@@ -212,9 +206,6 @@ public:
 
     bool isOpen() const;
 
-    // HttpControlMsgSink API
-    virtual void sendControlMsg(HttpControlMsg msg);
-
     Http1::TeChunkedParser *bodyParser; ///< parses HTTP/1.1 chunked request body
 
     /** number of body bytes we need to comm_read for the "current" request
@@ -393,7 +384,7 @@ public:
     void connectionTag(const char *aTag) { connectionTag_ = aTag; }
 
     /// handle a control message received by context from a peer and call back
-    virtual void writeControlMsgAndCall(ClientSocketContext *context, HttpReply *rep, AsyncCall::Pointer &call) = 0;
+    virtual void writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call) = 0;
 
     /// ClientStream calls this to supply response header (once) and data
     /// for the current ClientSocketContext.

diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index 358ea70d3485a5128fc8128625e1f7ea53466efc..1b25901a1eedda2080e34ae964492a8522470971 100644 (file)
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -1135,7 +1135,7 @@ Ftp::Server::writeForwardedForeign(const HttpReply *reply)
 }
 
 void
-Ftp::Server::writeControlMsgAndCall(ClientSocketContext *, HttpReply *reply, AsyncCall::Pointer &call)
+Ftp::Server::writeControlMsgAndCall(HttpReply *reply, AsyncCall::Pointer &call)
 {
     // the caller guarantees that we are dealing with the current context only
     // the caller should also make sure reply->header.has(Http::HdrType::FTP_STATUS)

diff --git a/src/servers/FtpServer.h b/src/servers/FtpServer.h
index 7d288a79b9dc58c0eb896c768cbfd361c2718fa0..8de7fd5ec7fc7e61598f9bb38049bfe471990674 100644 (file)
--- a/src/servers/FtpServer.h
+++ b/src/servers/FtpServer.h
@@ -87,7 +87,7 @@ protected:
     virtual void clientPinnedConnectionClosed(const CommCloseCbParams &io);
     virtual void handleReply(HttpReply *header, StoreIOBuffer receivedData);
     virtual int pipelinePrefetchMax() const;
-    virtual void writeControlMsgAndCall(ClientSocketContext *context, HttpReply *rep, AsyncCall::Pointer &call);
+    virtual void writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call);
     virtual time_t idleTimeout() const;
 
     /* BodyPipe API */

diff --git a/src/servers/Http1Server.cc b/src/servers/Http1Server.cc
index b03e65e1755cf1a5879f11e8ed19ac2efb2cf9ad..3b17df8290506e8de35c6665f1d1de9b9c703238 100644 (file)
--- a/src/servers/Http1Server.cc
+++ b/src/servers/Http1Server.cc
@@ -273,7 +273,7 @@ Http::One::Server::handleReply(HttpReply *rep, StoreIOBuffer receivedData)
 }
 
 void
-Http::One::Server::writeControlMsgAndCall(ClientSocketContext *context, HttpReply *rep, AsyncCall::Pointer &call)
+Http::One::Server::writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call)
 {
     // apply selected clientReplyContext::buildReplyHeader() mods
     // it is not clear what headers are required for control messages
@@ -286,7 +286,7 @@ Http::One::Server::writeControlMsgAndCall(ClientSocketContext *context, HttpRepl
     debugs(11, 2, "HTTP Client " << clientConnection);
     debugs(11, 2, "HTTP Client CONTROL MSG:\n---------\n" << mb->buf << "\n----------");
 
-    Comm::Write(context->clientConnection, mb, call);
+    Comm::Write(clientConnection, mb, call);
 
     delete mb;
 }

diff --git a/src/servers/Http1Server.h b/src/servers/Http1Server.h
index 5f3c196e05009784544847beaf4636ce52b0385b..1b042a626eed80e52a98372e7198a0c7715e30be 100644 (file)
--- a/src/servers/Http1Server.h
+++ b/src/servers/Http1Server.h
@@ -32,7 +32,7 @@ protected:
     virtual ClientSocketContext *parseOneRequest();
     virtual void processParsedRequest(ClientSocketContext *context);
     virtual void handleReply(HttpReply *rep, StoreIOBuffer receivedData);
-    virtual void writeControlMsgAndCall(ClientSocketContext *context, HttpReply *rep, AsyncCall::Pointer &call);
+    virtual void writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call);
     virtual time_t idleTimeout() const;
 
     /* BodyPipe API */

diff --git a/src/tests/Stub.list b/src/tests/Stub.list
index 0b4aab97954d1da64b4660ec37633932e0a8d25b..6e54a10048263d42e6885ce883d24eb3d476f4d0 100644 (file)
--- a/src/tests/Stub.list
+++ b/src/tests/Stub.list
@@ -32,6 +32,7 @@ STUB_SOURCE= tests/STUB.h \
        tests/stub_helper.cc \
        tests/stub_HelperChildConfig.cc \
        tests/stub_http.cc \
+       tests/stub_HttpControlMsg.cc \
        tests/stub_HttpReply.cc \
        tests/stub_HttpRequest.cc \
        tests/stub_icp.cc \

diff --git a/src/tests/stub_HttpControlMsg.cc b/src/tests/stub_HttpControlMsg.cc
new file mode 100644 (file)
index 0000000..0266bca
--- /dev/null
+++ b/src/tests/stub_HttpControlMsg.cc
@@ -0,0 +1,15 @@
+/*
+ * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#include "squid.h"
+
+#define STUB_API "HttpControlMsg.cc"
+#include "STUB.h"
+
+#include "HttpControlMsg.h"
+void HttpControlMsgSink::wroteControlMsg(CommIoCbParams const&) STUB

diff --git a/src/tests/stub_client_side.cc b/src/tests/stub_client_side.cc
index 0a7e9e2040d1b1cc975c84f04adba1d12a771b15..f143ab110e4409a8eea37137ef489bfc7dc0124a 100644 (file)
--- a/src/tests/stub_client_side.cc
+++ b/src/tests/stub_client_side.cc
@@ -32,7 +32,6 @@ void ClientSocketContext::deferRecipientForLater(clientStreamNode * node, HttpRe
 bool ClientSocketContext::multipartRangeRequest() const STUB_RETVAL(false)
 void ClientSocketContext::registerWithConn() STUB
 void ClientSocketContext::noteIoError(const int xerrno) STUB
-void ClientSocketContext::writeControlMsg(HttpControlMsg &msg) STUB
 
 bool ConnStateData::clientParseRequests() STUB_RETVAL(false)
 void ConnStateData::readNextRequest() STUB