From: Amos Jeffries Date: Mon, 30 Nov 2015 14:23:16 +0000 (-0800) Subject: Cleanup: Simplify HTTP 1xx control message writing X-Git-Tag: SQUID_4_0_4~59 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=84540b47bf1851c0ad4b9c1dcd69ae9ef3ccfff8;p=thirdparty%2Fsquid.git 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 index 0000000000..e3337015d5 --- /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 ¶ms) +{ + 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 f424157f3b..bb5c9b137a 100644 --- 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 f56a7e7bb3..7e752d37eb 100644 --- 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 350e568802..02894faf45 100644 --- 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(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 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 bce603db72..c6f6a040a5 100644 --- 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 358ea70d34..1b25901a1e 100644 --- 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 7d288a79b9..8de7fd5ec7 100644 --- 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 b03e65e175..3b17df8290 100644 --- 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 5f3c196e05..1b042a626e 100644 --- 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 0b4aab9795..6e54a10048 100644 --- 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 index 0000000000..0266bcaf6b --- /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 0a7e9e2040..f143ab110e 100644 --- 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