On Mon, 16 Aug 2010 15:53:42 -0600, Alex Rousskov
<rouss...@measurement-factory.com> wrote:
> Hello,
>
> We need to forward 1xx control messages from servers to clients. I
> see two implementation options:
>
> 1. Use Store. Squid client side expects responses via storeClientCopy,
> so we will be using the usual/normal code paths. Multiple 1xx responses
> may be handled with relative ease. The 1xx responses in Store will be
> treated kind of as regular response headers, except they will not be
> cached and such. The code will need to "skip" them until they reach the
> socket-writing client.
>
> 2. Bypass Store. Contact fwdStart caller (e.g., clientReplyContext)
> directly and give it a 1xx response to forward. Store code remains
> unchanged. It may be difficult to get from the fwdStart caller to the
> client socket and comm_write. It will be difficult to handle multiple
> 1xx responses or a regular response that arrives before we are done with
> writing 1xx response (all unusual, but can happen!).
>
> Both approaches may have to deal with crazy offset management,
> clientStreams manipulations, and other client-side mess.
>
>
> Do you see any other options? Which option is the best?
On 08/16/2010 04:06 PM, Amos Jeffries wrote:
My earlier plan if I did it was to do (2). The complication only occurs at
one point, finding the client FD.
comm_write() should not be altering offset of the higher level store stuff
directly. If it is that is a bug to be fixed.
Pipelining the responses one at a time with a simple block on further
reply passing-on until the existing header set has been finished with gets
around any trickiness with multiple or early real responses.
The "block on further reply passing" is far from simple because it needs
to deal with two async jobs.
On 08/18/2010 02:11 PM, Henrik Nordström wrote:
mån 2010-08-16 klockan 15:53 -0600 skrev Alex Rousskov:
Both approaches may have to deal with crazy offset management,
clientStreams manipulations, and other client-side mess.
Yes.
For now I think we need to bypass store to make this sane, and it's
probably also a step in the right direction in general.
Thank you both for your feedback!
The attached patch implements the "Bypass Store" design and forwards 1xx
control messages to clients that are likely to be able to handle such
messages.
The patch appears to pass initial tests but more testing and a sync with
trunk are needed. There is also one XXX that I still need to resolve,
but it requires some code from the bug #2583 (pure virtual call) fix. I
will switch to committing that fix now.
Meanwhile, if you have a chance, please review the overall direction of
the patch. Preamble has more notes.
The patch removes the ignore_expect_100 feature because we now forward
100 Continue messages. Is everybody OK with that removal?
Thank you,
Alex.
Compliance: Forward 1xx control messages to clients that support them.
Take 0, which needs more work.
The patch removes ignore_expect_100 squid.conf option because we can safely
forward Expect: 100-continue headers to servers because we can forward
100 Continue control messages to the expecting clients.
We now forward 1xx control messages to all HTTP/1.1 clients and to HTTP/1.0
clients that sent an Expect: 100-continue header. RFC 2616 requires clients
to accept 1xx control messages, even if they did not send Expect headers.
We still respond with 417 Expectation Failed to requests with expectations
other than 100-continue.
Implementation notes:
We forward control messages one-at-a-time and stop processing the server
response while the 1xx message is being written to client, to avoid
server-driven DoS attacks with large number of 1xx messages.
1xx forwarding is done via async calls from HttpStateData to
ConnStateData/ClientSocketContext. The latter then calls back to notify
HttpStateData that the message was written out to client. If any one of the
two async messages is not fired, HttpStateData will get stuck unless it is
destroyed due to an external event/error. The code assumes such event/error
will always happen because when ConnStateData/ClientSocketContext is gone,
HttpStateData job should be terminated. This requires more testing/thought.
XXX: The patch is not finished. We need to cbdata-protect the
HttpRequest::clientConnection member and re-sync with trunk.
=== added file 'src/HttpControlMsg.h'
--- src/HttpControlMsg.h 1970-01-01 00:00:00 +0000
+++ src/HttpControlMsg.h 2010-08-18 19:36:04 +0000
@@ -0,0 +1,57 @@
+/*
+ * $Id$
+ */
+
+#ifndef SQUID_HTTP_CONTROL_MSG_H
+#define SQUID_HTTP_CONTROL_MSG_H
+
+#include "HttpReply.h"
+#include "base/AsyncCall.h"
+
+class HttpControlMsg;
+
+/*
+ * This API exists to throttle forwarding of 1xx messages from the server
+ * side (Source == HttpStateData) to the client side (Sink == ConnStateData).
+ *
+ * Without throttling, Squid would have to drop some 1xx responses to
+ * avoid DoS attacks that send many 1xx responses without reading them.
+ * Dropping 1xx responses without violating HTTP is as complex as throttling.
+ */
+
+/// sends a single control message, notifying the Sink
+class HttpControlMsgSink: public virtual AsyncJob
+{
+public:
+ HttpControlMsgSink(): AsyncJob("unused") {}
+
+ /// called to send the 1xx message and notify the Source
+ virtual void sendControlMsg(HttpControlMsg msg) = 0;
+};
+
+/// bundles HTTP 1xx reply and the "successfully forwarded" callback
+class HttpControlMsg
+{
+public:
+ typedef HttpMsgPointerT<HttpReply> MsgPtr;
+ typedef AsyncCall::Pointer Callback;
+
+ HttpControlMsg(const MsgPtr &aReply, const Callback &aCallback):
+ reply(aReply), cbSuccess(aCallback) {}
+
+public:
+ MsgPtr reply; ///< the 1xx message being forwarded
+ Callback cbSuccess; ///< called after successfully writing the 1xx message
+
+ // We could add an API to notify of send failures as well, but the
+ // current Source and Sink are tied via Store anyway, so the Source
+ // will know, eventually, if the Sink is gone or otherwise failed.
+};
+
+inline std::ostream &
+operator <<(std::ostream &os, const HttpControlMsg &msg)
+{
+ return os << msg.reply << ", " << msg.cbSuccess;
+}
+
+#endif /* SQUID_HTTP_CONTROL_MSG_H */
=== modified file 'src/HttpMsg.h'
--- src/HttpMsg.h 2010-01-01 21:16:57 +0000
+++ src/HttpMsg.h 2010-08-19 04:48:27 +0000
@@ -159,4 +159,41 @@
#define HTTPMSGUNLOCK(a) if(a){(a)->_unlock();(a)=NULL;}
#define HTTPMSGLOCK(a) (a)->_lock()
+// TODO: replace HTTPMSGLOCK with general RefCounting and delete this class
+/// safe HttpMsg pointer wrapper that locks and unlocks the message
+template <class Msg>
+class HttpMsgPointerT
+{
+public:
+ HttpMsgPointerT(): msg(NULL) {}
+ explicit HttpMsgPointerT(Msg *m): msg(m) { lock(); }
+ virtual ~HttpMsgPointerT() { unlock(); }
+
+ HttpMsgPointerT(const HttpMsgPointerT &p): msg(p.msg) { lock(); }
+ HttpMsgPointerT &operator =(const HttpMsgPointerT &p)
+ { if (msg != p.msg) { unlock(); msg = p.msg; lock(); } return *this; }
+
+ Msg &operator *() { return *msg; }
+ const Msg &operator *() const { return *msg; }
+ Msg *operator ->() { return msg; }
+ const Msg *operator ->() const { return msg; }
+ operator Msg *() { return msg; }
+ operator const Msg *() const { return msg; }
+ // add more as needed
+
+ void lock() { if (msg) HTTPMSGLOCK(msg); } ///< prevent msg destruction
+ void unlock() { HTTPMSGUNLOCK(msg); } ///< allows/causes msg destruction
+
+private:
+ Msg *msg;
+};
+
+/// convenience wrapper to create HttpMsgPointerT<> object based on msg type
+template <class Msg>
+inline
+HttpMsgPointerT<Msg> HttpMsgPointer(Msg *msg)
+{
+ return HttpMsgPointerT<Msg>(msg);
+}
+
#endif /* SQUID_HTTPMSG_H */
=== modified file 'src/HttpRequest.cc'
--- src/HttpRequest.cc 2010-07-13 16:43:00 +0000
+++ src/HttpRequest.cc 2010-08-17 20:59:49 +0000
@@ -644,3 +644,16 @@
return rangeOffsetLimit;
}
+
+bool
+HttpRequest::canHandle1xx() const
+{
+ // old clients do not support 1xx by default
+ if (http_ver <= HttpVersion(1,0) && !header.has(HDR_EXPECT))
+ return false;
+
+ // TODO: add configurable exceptions here
+
+ // others must support 1xx
+ return true;
+}
=== modified file 'src/HttpRequest.h'
--- src/HttpRequest.h 2010-04-17 10:38:50 +0000
+++ src/HttpRequest.h 2010-08-18 16:17:43 +0000
@@ -88,6 +88,9 @@
/* are responses to this request potentially cachable */
bool cacheable() const;
+ /// whether the client is likely to be able to handle a 1xx reply
+ bool canHandle1xx() const;
+
/* Now that we care what host contains it is better off being protected. */
/* HACK: These two methods are only inline to get around Makefile dependancies */
/* caused by HttpRequest being used in places it really shouldn't. */
@@ -248,6 +251,9 @@
cbdataReferenceDone(pinned_connection);
}
+ // XXX: needs cbdataReference protection!
+ ConnStateData *clientConnection; ///< client-side conn manager, if known
+
int64_t getRangeOffsetLimit(); /* the result of this function gets cached in rangeOffsetLimit */
private:
=== modified file 'src/HttpVersion.h'
--- src/HttpVersion.h 2009-01-21 03:47:47 +0000
+++ src/HttpVersion.h 2010-08-17 17:47:55 +0000
@@ -66,6 +66,23 @@
return ((this->major != that.major) || (this->minor != that.minor));
}
+ bool operator <(const HttpVersion& that) const {
+ return (this->major < that.major ||
+ this->major == that.major && this->minor < that.minor);
+ }
+
+ bool operator >(const HttpVersion& that) const {
+ return (this->major > that.major ||
+ this->major == that.major && this->minor > that.minor);
+ }
+
+ bool operator <=(const HttpVersion& that) const {
+ return !(*this > that);
+ }
+
+ bool operator >=(const HttpVersion& that) const {
+ return !(*this < that);
+ }
};
#endif /* SQUID_HTTPVERSION_H */
=== modified file 'src/Makefile.am'
--- src/Makefile.am 2010-08-16 21:20:53 +0000
+++ src/Makefile.am 2010-08-19 04:47:30 +0000
@@ -354,6 +354,7 @@
HttpBody.cc \
HttpMsg.cc \
HttpMsg.h \
+ HttpControlMsg.h \
HttpReply.cc \
HttpReply.h \
HttpRequest.cc \
=== modified file 'src/cf.data.pre'
--- src/cf.data.pre 2010-08-14 16:38:27 +0000
+++ src/cf.data.pre 2010-08-19 05:37:57 +0000
@@ -4164,21 +4164,6 @@
or response to be rejected.
DOC_END
-NAME: ignore_expect_100
-COMMENT: on|off
-IFDEF: USE_HTTP_VIOLATIONS
-TYPE: onoff
-LOC: Config.onoff.ignore_expect_100
-DEFAULT: off
-DOC_START
- This option makes Squid ignore any Expect: 100-continue header present
- in the request. RFC 2616 requires that Squid being unable to satisfy
- the response expectation MUST return a 417 error.
-
- Note: Enabling this is a HTTP protocol violation, but some clients may
- not handle it well..
-DOC_END
-
COMMENT_START
TIMEOUTS
-----------------------------------------------------------------------------
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2010-08-07 14:22:54 +0000
+++ src/client_side.cc 2010-08-19 05:32:19 +0000
@@ -342,6 +342,59 @@
return newContext;
}
+void
+ClientSocketContext::writeControlMsg(HttpControlMsg &msg)
+{
+ HttpReply *rep = msg.reply;
+ Must(rep);
+
+ // apply selected clientReplyContext::buildReplyHeader() mods
+ // it is not clear what headers are required for control messages
+ rep->header.removeHopByHopEntries();
+ rep->header.putStr(HDR_CONNECTION, "keep-alive");
+ httpHdrMangleList(&rep->header, http->request, ROR_REPLY);
+
+ // remember the callback
+ cbControlMsgSent = msg.cbSuccess;
+
+ MemBuf *mb = rep->pack();
+
+ AsyncCall::Pointer call = commCbCall(33, 5, "ClientSocketContext::wroteControlMsg",
+ CommIoCbPtrFun(&WroteControlMsg, this));
+ comm_write_mbuf(fd(), mb, call);
+
+ delete mb;
+}
+
+/// called when we wrote the 1xx response
+void
+ClientSocketContext::wroteControlMsg(int fd, char *, size_t, comm_err_t 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!)
+ comm_close(fd);
+}
+
+/// wroteControlMsg() wrapper: ClientSocketContext is not an AsyncJob
+void
+ClientSocketContext::WroteControlMsg(int fd, char *bufnotused, size_t size, comm_err_t errflag, int xerrno, void *data)
+{
+ ClientSocketContext *context = static_cast<ClientSocketContext*>(data);
+ context->wroteControlMsg(fd, bufnotused, size, errflag, xerrno);
+}
+
#if USE_IDENT
static void
clientIdentDone(const char *ident, void *data)
@@ -742,10 +795,7 @@
debugs(33, 3, "clientSetKeepaliveFlag: method = " <<
RequestMethodStr(request->method));
- /* We are HTTP/1.1 facing clients now*/
- HttpVersion http_ver(1,1);
-
- if (httpMsgIsPersistent(http_ver, req_hdr))
+ if (httpMsgIsPersistent(request->http_ver, req_hdr))
request->flags.proxy_keepalive = 1;
}
@@ -2527,16 +2577,9 @@
}
if (request->header.has(HDR_EXPECT)) {
- int ignore = 0;
-#if USE_HTTP_VIOLATIONS
- if (Config.onoff.ignore_expect_100) {
- String expect = request->header.getList(HDR_EXPECT);
- if (expect.caseCmp("100-continue") == 0)
- ignore = 1;
- expect.clean();
- }
-#endif
- if (!ignore) {
+ const String expect = request->header.getList(HDR_EXPECT);
+ const bool supportedExpect = (expect.caseCmp("100-continue") == 0);
+ if (!supportedExpect) {
clientStreamNode *node = context->getClientReplyContext();
clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
assert (repContext);
@@ -3859,6 +3902,24 @@
delete bodyParser; // TODO: pool
}
+void
+ConnStateData::sendControlMsg(HttpControlMsg msg)
+{
+ ClientSocketContext::Pointer context = getCurrentContext();
+ if (context != NULL) {
+ context->writeControlMsg(msg); // will call msg.cbSuccess
+ return;
+ }
+
+ if (!isOpen()) {
+ debugs(33, 3, HERE << "ignoring 1xx due to earlier closure");
+ return;
+ }
+
+ debugs(33, 3, HERE << " closing due to missing context for 1xx");
+ comm_close(fd);
+}
+
/* This is a comm call normally scheduled by comm_close() */
void
ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io)
=== modified file 'src/client_side.h'
--- src/client_side.h 2010-07-30 20:11:20 +0000
+++ src/client_side.h 2010-08-19 04:59:25 +0000
@@ -42,6 +42,7 @@
#include "eui/Eui64.h"
#include "RefCount.h"
#include "StoreIOBuffer.h"
+#include "HttpControlMsg.h"
class ConnStateData;
class ClientHttpRequest;
@@ -112,6 +113,13 @@
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 void WroteControlMsg(int fd, char *bufnotused, size_t size, comm_err_t errflag, int xerrno, void *data);
+ void wroteControlMsg(int fd, char *bufnotused, size_t size, comm_err_t errflag, int xerrno);
+
private:
CBDATA_CLASS(ClientSocketContext);
void prepareReply(HttpReply * rep);
@@ -119,6 +127,9 @@
void deRegisterWithConn();
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_;
};
@@ -127,7 +138,7 @@
class ConnectionDetail;
/** A connection to a socket */
-class ConnStateData : public BodyProducer/*, public RefCountable*/
+class ConnStateData : public BodyProducer, public HttpControlMsgSink
{
public:
@@ -147,6 +158,9 @@
int getConcurrentRequestCount() const;
bool isOpen() const;
+ // HttpControlMsgSink API
+ virtual void sendControlMsg(HttpControlMsg msg);
+
int fd;
/// chunk buffering and parsing algorithm state
=== modified file 'src/client_side_reply.cc'
--- src/client_side_reply.cc 2010-08-14 02:58:39 +0000
+++ src/client_side_reply.cc 2010-08-18 16:56:17 +0000
@@ -261,6 +261,8 @@
http->storeEntry(entry);
assert(http->out.offset == 0);
+ http->request->clientConnection = http->getConn();
+
/*
* A refcounted pointer so that FwdState stays around as long as
* this clientReplyContext does
@@ -680,6 +682,8 @@
if (http->flags.internal)
r->protocol = PROTO_INTERNAL;
+ r->clientConnection = http->getConn();
+
/** Start forwarding to get the new object from network */
FwdState::fwdStart(http->getConn() != NULL ? http->getConn()->fd : -1,
http->storeEntry(),
=== modified file 'src/http.cc'
--- src/http.cc 2010-08-13 07:53:08 +0000
+++ src/http.cc 2010-08-19 15:38:15 +0000
@@ -54,6 +54,7 @@
#include "HttpHdrScTarget.h"
#include "HttpReply.h"
#include "HttpRequest.h"
+#include "HttpControlMsg.h"
#include "MemBuf.h"
#include "MemObject.h"
#include "protos.h"
@@ -705,23 +706,9 @@
readBuf->consume(header_bytes_read);
}
- /* Skip 1xx messages for now. Advertised in Via as an internal 1.0 hop */
if (newrep->sline.protocol == PROTO_HTTP && newrep->sline.status >= 100 && newrep->sline.status < 200) {
-
-#if WHEN_HTTP11_EXPECT_HANDLED
- /* When HTTP/1.1 check if the client is expecting a 1xx reply and maybe pass it on */
- if (orig_request->header.has(HDR_EXPECT)) {
- // TODO: pass to the client anyway?
- }
-#endif
- delete newrep;
- debugs(11, 2, HERE << "1xx headers consume " << header_bytes_read << " bytes header.");
- header_bytes_read = 0;
- if (reply_bytes_read > 0)
- debugs(11, 2, HERE << "1xx headers consume " << reply_bytes_read << " bytes reply.");
- reply_bytes_read = 0;
+ handle1xx(newrep);
ctx_exit(ctx);
- processReplyHeader();
return;
}
@@ -752,6 +739,57 @@
ctx_exit(ctx);
}
+/// ignore or start forwarding the 1xx response (a.k.a., control message)
+void
+HttpStateData::handle1xx(HttpReply *reply)
+{
+ HttpMsgPointerT<HttpReply> msg(reply); // will destroy reply if unused
+
+ // one 1xx at a time: we must not be called while waiting for previous 1xx
+ Must(!flags.handling1xx);
+ flags.handling1xx = true;
+
+ if (!orig_request->canHandle1xx()) {
+ debugs(11, 2, HERE << "ignoring client-unsupported 1xx");
+ proceedAfter1xx();
+ return;
+ }
+
+ debugs(11, 2, HERE << "forwarding 1xx to client");
+
+ // the Sink will use this to call us back after writing 1xx to the client
+ typedef NullaryMemFunT<HttpStateData> CbDialer;
+ AsyncCall::Pointer cb = asyncCall(11, 3, "HttpStateData::proceedAfter1xx",
+ CbDialer(this, &HttpStateData::proceedAfter1xx));
+
+ typedef UnaryMemFunT<ConnStateData, HttpControlMsg> SinkDialer;
+ ConnStateData *conn = orig_request->clientConnection;
+ AsyncCall::Pointer call = asyncCall(11, 4, "ConnStateData::sendControlMessage",
+ SinkDialer(conn, &ConnStateData::sendControlMsg, HttpControlMsg(msg, cb)));
+ ScheduleCallHere(call);
+ // If the call is not fired, then the Sink is gone, and HttpStateData
+ // will terminate due to an aborted store entry or another similar error.
+ // If we get stuck, it is not handle1xx fault, because we could get stuck
+ // for similar reasons without a 1xx response!
+}
+
+/// restores state and resumes processing after 1xx is ignored or forwarded
+void
+HttpStateData::proceedAfter1xx()
+{
+ Must(flags.handling1xx);
+
+ debugs(11, 2, HERE << "consuming " << header_bytes_read <<
+ " header and " << reply_bytes_read << " body bytes read after 1xx");
+ header_bytes_read = 0;
+ reply_bytes_read = 0;
+
+ AsyncCall::Pointer call = asyncCall(11, 3, "HttpStateData::processReply",
+ MemFun(this, &HttpStateData::processReply));
+ ScheduleCallHere(call);
+}
+
+
/**
* returns true if the peer can support connection pinning
*/
@@ -1132,6 +1170,20 @@
}
}
+ processReply();
+}
+
+/// processes the already read and buffered response data, possibly after
+/// waiting for asynchronous 1xx control message processing
+void
+HttpStateData::processReply() {
+
+ if (flags.handling1xx) { // we came back after handling a 1xx response
+ debugs(11, 5, HERE << "done with 1xx handling");
+ flags.handling1xx = false;
+ Must(!flags.headers_parsed);
+ }
+
if (!flags.headers_parsed) { // have not parsed headers yet?
PROF_start(HttpStateData_processReplyHeader);
processReplyHeader();
@@ -1155,6 +1207,12 @@
bool
HttpStateData::continueAfterParsingHeader()
{
+ if (flags.handling1xx) {
+ debugs(11, 5, HERE << "wait for 1xx handling");
+ Must(!flags.headers_parsed);
+ return false;
+ }
+
if (!flags.headers_parsed && !eof) {
debugs(11, 9, HERE << "needs more at " << readBuf->contentSize());
flags.do_next_read = 1;
=== modified file 'src/http.h'
--- src/http.h 2010-07-28 18:04:45 +0000
+++ src/http.h 2010-08-19 05:11:26 +0000
@@ -81,6 +81,10 @@
protected:
virtual HttpRequest *originalRequest();
+ void processReply();
+ void proceedAfter1xx();
+ void handle1xx(HttpReply *msg);
+
private:
AsyncCall::Pointer closeHandler;
enum ConnectionStatus {
=== modified file 'src/structs.h'
--- src/structs.h 2010-08-07 14:22:54 +0000
+++ src/structs.h 2010-08-17 21:16:01 +0000
@@ -760,6 +760,7 @@
unsigned int proxying:1;
unsigned int keepalive:1;
unsigned int only_if_cached:1;
+ unsigned int handling1xx:1; ///< we are ignoring or forwarding 1xx response
unsigned int headers_parsed:1;
unsigned int front_end_https:2;
unsigned int originpeer:1;
