1 ------------------------------------------------------------
3 revision-id: squid3@treenet.co.nz-20170701120848-q2xznzfvxb4kwvb6
4 parent: squid3@treenet.co.nz-20170701095916-wknqmneq2w0mxt6a
5 fixes bug: http://bugs.squid-cache.org/show_bug.cgi?id=4464
6 author: Christos Tsantilas <chtsanti@users.sourceforge.net>
7 committer: Amos Jeffries <squid3@treenet.co.nz>
9 timestamp: Sun 2017-07-02 00:08:48 +1200
11 Bug 4464: Reduce "!Comm::MonitorsRead(serverConnection->fd)" assertions.
13 * Protect Squid Client classes from new requests that compete with
14 ongoing pinned connection use and
15 * resume dealing with new requests when those Client classes are done
16 using the pinned connection.
18 Replaced primary ConnStateData::pinConnection() calls with a pair of
19 pinBusyConnection() and notePinnedConnectionBecameIdle() calls,
20 depending on the pinned connection state ("busy" or "idle").
22 Removed pinConnection() parameters that were not longer used or could be computed from the remaining parameters.
24 Removed ConnStateData::httpsPeeked() code "hiding" the originating
25 request and connection peer details while entering the first "idle"
26 state. The old (trunk r11880.1.6) bump-server-first code used a pair of
27 NULLs because "Intercepted connections do not have requests at the
28 connection pinning stage", but that limitation no longer applicable
29 because Squid always fakes (when intercepting) or parses (a CONNECT)
30 request now, even during SslBump step1.
32 The added XXX and TODOs are not directly related to this fix. They
33 were added to document problems discovered while working on this fix.
35 In v3.5 code, the same problems manifest as Read.cc
36 "fd_table[conn->fd].halfClosedReader != NULL" assertions.
38 This is a Measurement Factory project
39 ------------------------------------------------------------
40 # Bazaar merge directive format 2 (Bazaar 0.90)
41 # revision_id: squid3@treenet.co.nz-20170701120848-q2xznzfvxb4kwvb6
42 # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5
43 # testament_sha1: e4f432eed8a845431d4bbbf023de04d682adeaff
44 # timestamp: 2017-07-01 12:32:26 +0000
45 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5
46 # base_revision_id: squid3@treenet.co.nz-20170701095916-\
50 === modified file 'src/FwdState.cc'
51 --- src/FwdState.cc 2017-01-01 00:16:45 +0000
52 +++ src/FwdState.cc 2017-07-01 12:08:48 +0000
55 if (request->flags.sslPeek && request->clientConnectionManager.valid()) {
56 CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData,
57 - ConnStateData::httpsPeeked, Comm::ConnectionPointer(NULL));
58 + ConnStateData::httpsPeeked, ConnStateData::PinnedIdleContext(Comm::ConnectionPointer(nullptr), request));
64 if (request->flags.sslPeek) {
65 CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData,
66 - ConnStateData::httpsPeeked, serverConnection());
67 + ConnStateData::httpsPeeked, ConnStateData::PinnedIdleContext(serverConnection(), request));
68 unregister(serverConn); // async call owns it now
69 complete(); // destroys us
72 === modified file 'src/base/RefCount.h'
73 --- src/base/RefCount.h 2017-01-01 00:16:45 +0000
74 +++ src/base/RefCount.h 2017-07-01 12:08:48 +0000
77 C & operator * () const {return *const_cast<C *>(p_); }
79 - C const * getRaw() const {return p_; }
81 - C * getRaw() {return const_cast<C *>(p_); }
82 + C * getRaw() const { return const_cast<C *>(p_); }
84 bool operator == (const RefCount& p) const {
87 === modified file 'src/client_side.cc'
88 --- src/client_side.cc 2017-05-29 13:15:55 +0000
89 +++ src/client_side.cc 2017-07-01 12:08:48 +0000
91 assert(areAllContextsForThisConnection());
94 + // XXX: Closing pinned conn is too harsh: The Client may want to continue!
95 unpinConnection(true);
97 if (Comm::IsConnOpen(clientConnection))
98 @@ -1559,6 +1560,13 @@
100 debugs(33, 3, HERE << "ConnnStateData(" << conn->clientConnection << "), Context(" << clientConnection << ")");
106 +ConnStateData::kick()
108 + ConnStateData * conn = this; // XXX: Remove this diff minimization hack
110 if (conn->pinning.pinned && !Comm::IsConnOpen(conn->pinning.serverConnection)) {
111 debugs(33, 2, HERE << conn->clientConnection << " Connection was pinned but server side gone. Terminating client connection");
112 @@ -3240,6 +3248,13 @@
113 if (in.buf.isEmpty())
116 + // Prohibit concurrent requests when using a pinned to-server connection
117 + // because our Client classes do not support request pipelining.
118 + if (pinning.pinned && !pinning.readHandler) {
119 + debugs(33, 3, clientConnection << " waits for busy " << pinning.serverConnection);
123 /* Limit the number of concurrent requests */
124 if (concurrentRequestQueueFilled())
126 @@ -4434,22 +4449,19 @@
130 -ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection)
131 +ConnStateData::httpsPeeked(PinnedIdleContext pic)
133 Must(sslServerBump != NULL);
134 + Must(sslServerBump->request == pic.request);
135 + Must(currentobject == NULL || currentobject->http == NULL || currentobject->http->request == pic.request.getRaw());
137 - if (Comm::IsConnOpen(serverConnection)) {
138 - pinConnection(serverConnection, NULL, NULL, false);
139 + if (Comm::IsConnOpen(pic.connection)) {
140 + notePinnedConnectionBecameIdle(pic);
142 debugs(33, 5, HERE << "bumped HTTPS server: " << sslConnectHostOrIp);
145 debugs(33, 5, HERE << "Error while bumping: " << sslConnectHostOrIp);
147 - // copy error detail from bump-server-first request to CONNECT request
148 - if (currentobject != NULL && currentobject->http != NULL && currentobject->http->request)
149 - currentobject->http->request->detailError(sslServerBump->request->errType, sslServerBump->request->errDetail);
152 getSslContextStart();
155 @@ -4952,19 +4964,35 @@
159 -ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth, bool monitor)
161 - if (!Comm::IsConnOpen(pinning.serverConnection) ||
162 - pinning.serverConnection->fd != pinServer->fd)
163 - pinNewConnection(pinServer, request, aPeer, auth);
166 - startPinnedConnectionMonitoring();
170 -ConnStateData::pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth)
172 +ConnStateData::pinBusyConnection(const Comm::ConnectionPointer &pinServer, const HttpRequest::Pointer &request)
174 + pinConnection(pinServer, request);
178 +ConnStateData::notePinnedConnectionBecameIdle(PinnedIdleContext pic)
180 + Must(pic.connection != NULL);
181 + Must(pic.request != NULL);
182 + pinConnection(pic.connection, pic.request);
184 + // monitor pinned server connection for remote-end closures.
185 + startPinnedConnectionMonitoring();
187 + if (!currentobject)
188 + kick(); // in case clientParseRequests() was blocked by a busy pic.connection
191 +/// Forward future client requests using the given server connection.
193 +ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, const HttpRequest::Pointer &request)
195 + if (Comm::IsConnOpen(pinning.serverConnection) &&
196 + pinning.serverConnection->fd == pinServer->fd) {
197 + debugs(33, 3, "already pinned" << pinServer);
201 unpinConnection(true); // closes pinned connection, if any, and resets fields
203 pinning.serverConnection = pinServer;
204 @@ -4973,23 +5001,21 @@
206 Must(pinning.serverConnection != NULL);
208 - // when pinning an SSL bumped connection, the request may be NULL
209 const char *pinnedHost = "[unknown]";
211 + if (request != NULL) {
212 pinning.host = xstrdup(request->GetHost());
213 pinning.port = request->port;
214 pinnedHost = pinning.host;
215 + pinning.auth = request->flags.connectionAuth;
217 pinning.port = pinServer->remote.port();
219 pinning.pinned = true;
221 - pinning.peer = cbdataReference(aPeer);
222 - pinning.auth = auth;
223 + pinning.peer = cbdataReference(pinServer->getPeer());
224 char stmp[MAX_IPSTRLEN];
225 char desc[FD_DESC_SZ];
226 snprintf(desc, FD_DESC_SZ, "%s pinned connection for %s (%d)",
227 - (auth || !aPeer) ? pinnedHost : aPeer->name,
228 + (pinning.auth || !pinning.peer) ? pinnedHost : pinning.peer->name,
229 clientConnection->remote.toUrl(stmp,MAX_IPSTRLEN),
230 clientConnection->fd);
231 fd_note(pinning.serverConnection->fd, desc);
232 @@ -5164,3 +5190,8 @@
233 * connection has gone away */
237 +operator <<(std::ostream &os, const ConnStateData::PinnedIdleContext &pic)
239 + return os << pic.connection << ", request=" << pic.request;
242 === modified file 'src/client_side.h'
243 --- src/client_side.h 2017-01-01 00:16:45 +0000
244 +++ src/client_side.h 2017-07-01 12:08:48 +0000
246 #include "ssl/support.h"
252 class ClientHttpRequest;
253 class clientStreamNode;
256 bool clientParseRequests();
257 void readNextRequest();
259 + // In v3.5, usually called via ClientSocketContext::keepaliveNextRequest().
260 + /// try to make progress on a transaction or read more I/O
263 ClientSocketContext::Pointer getCurrentContext() const;
264 void addContextToQueue(ClientSocketContext * context);
265 int getConcurrentRequestCount() const;
267 bool handleReadData();
268 bool handleRequestBodyData();
270 - /// Forward future client requests using the given server connection.
271 - /// Optionally, monitor pinned server connection for remote-end closures.
272 - void pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, CachePeer *peer, bool auth, bool monitor = true);
273 + /// parameters for the async notePinnedConnectionBecameIdle() call
274 + class PinnedIdleContext
277 + PinnedIdleContext(const Comm::ConnectionPointer &conn, const HttpRequest::Pointer &req): connection(conn), request(req) {}
279 + Comm::ConnectionPointer connection; ///< to-server connection to be pinned
280 + HttpRequest::Pointer request; ///< to-server request that initiated serverConnection
283 + /// Called when a pinned connection becomes available for forwarding the next request.
284 + void notePinnedConnectionBecameIdle(PinnedIdleContext pic);
285 + /// Forward future client requests using the given to-server connection.
286 + /// The connection is still being used by the current client request.
287 + void pinBusyConnection(const Comm::ConnectionPointer &pinServerConn, const HttpRequest::Pointer &request);
288 /// Undo pinConnection() and, optionally, close the pinned connection.
289 void unpinConnection(const bool andClose);
290 /// Returns validated pinnned server connection (and stops its monitoring).
293 void doPeekAndSpliceStep();
294 /// called by FwdState when it is done bumping the server
295 - void httpsPeeked(Comm::ConnectionPointer serverConnection);
296 + void httpsPeeked(PinnedIdleContext pic);
298 /// Start to create dynamic SSL_CTX for host or uses static port SSL context.
299 void getSslContextStart();
301 void clientAfterReadingRequests();
302 bool concurrentRequestQueueFilled() const;
304 - void pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth);
305 + void pinConnection(const Comm::ConnectionPointer &pinServerConn, const HttpRequest::Pointer &request);
307 /* PROXY protocol functionality */
308 bool proxyProtocolValidateClient();
310 void clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *context, const HttpRequestMethod& method, Http::ProtocolVersion http_ver);
311 void clientPostHttpsAccept(ConnStateData *connState);
313 +std::ostream &operator <<(std::ostream &os, const ConnStateData::PinnedIdleContext &pic);
315 #endif /* SQUID_CLIENTSIDE_H */
318 === modified file 'src/clients/FtpRelay.cc'
319 --- src/clients/FtpRelay.cc 2017-01-01 00:16:45 +0000
320 +++ src/clients/FtpRelay.cc 2017-07-01 12:08:48 +0000
322 mgr->unpinConnection(false);
325 - mgr->pinConnection(ctrl.conn, fwd->request,
326 - ctrl.conn->getPeer(),
327 - fwd->request->flags.connectionAuth);
328 + CallJobHere1(9, 4, mgr,
330 + notePinnedConnectionBecameIdle,
331 + ConnStateData::PinnedIdleContext(ctrl.conn, fwd->request));
336 === modified file 'src/http.cc'
337 --- src/http.cc 2017-06-15 00:16:33 +0000
338 +++ src/http.cc 2017-07-01 12:08:48 +0000
339 @@ -1383,9 +1383,6 @@
341 HttpStateData::processReplyBody()
343 - Ip::Address client_addr;
344 - bool ispinned = false;
346 if (!flags.headers_parsed) {
347 flags.do_next_read = true;
348 maybeReadVirginBody();
349 @@ -1435,35 +1432,49 @@
353 - case COMPLETE_PERSISTENT_MSG:
354 + case COMPLETE_PERSISTENT_MSG: {
355 debugs(11, 5, "processReplyBody: COMPLETE_PERSISTENT_MSG from " << serverConnection);
356 - /* yes we have to clear all these! */
358 + // TODO: Remove serverConnectionSaved but preserve exception safety.
360 commUnsetConnTimeout(serverConnection);
361 flags.do_next_read = false;
363 comm_remove_close_handler(serverConnection->fd, closeHandler);
365 - fwd->unregister(serverConnection);
367 + Ip::Address client_addr; // XXX: Remove as unused. Why was it added?
368 if (request->flags.spoofClientIp)
369 client_addr = request->client_addr;
371 + Comm::ConnectionPointer serverConnectionSaved = serverConnection;
372 + fwd->unregister(serverConnection);
373 + serverConnection = nullptr;
375 + bool ispinned = false; // TODO: Rename to isOrShouldBePinned
376 if (request->flags.pinned) {
378 } else if (request->flags.connectionAuth && request->flags.authSent) {
382 - if (ispinned && request->clientConnectionManager.valid()) {
383 - request->clientConnectionManager->pinConnection(serverConnection, request, _peer,
384 - (request->flags.connectionAuth));
386 + if (request->clientConnectionManager.valid()) {
387 + CallJobHere1(11, 4, request->clientConnectionManager,
389 + notePinnedConnectionBecameIdle,
390 + ConnStateData::PinnedIdleContext(serverConnectionSaved, request));
392 + // must not pool/share ispinned connections, even orphaned ones
393 + serverConnectionSaved->close();
396 - fwd->pconnPush(serverConnection, request->GetHost());
397 + fwd->pconnPush(serverConnectionSaved, request->GetHost());
400 - serverConnection = NULL;
405 case COMPLETE_NONPERSISTENT_MSG:
406 debugs(11, 5, "processReplyBody: COMPLETE_NONPERSISTENT_MSG from " << serverConnection);
408 === modified file 'src/servers/FtpServer.cc'
409 --- src/servers/FtpServer.cc 2017-02-26 11:09:42 +0000
410 +++ src/servers/FtpServer.cc 2017-07-01 12:08:48 +0000
413 HttpRequest *const request = http->request;
414 Must(request != NULL);
416 - // this is not an idle connection, so we do not want I/O monitoring
417 - const bool monitor = false;
419 // make FTP peer connection exclusive to our request
420 - pinConnection(conn, request, conn->getPeer(), false, monitor);
421 + pinBusyConnection(conn, request);
426 === modified file 'src/tests/stub_client_side.cc'
427 --- src/tests/stub_client_side.cc 2017-01-01 00:16:45 +0000
428 +++ src/tests/stub_client_side.cc 2017-07-01 12:08:48 +0000
430 void ConnStateData::noteBodyConsumerAborted(BodyPipe::Pointer) STUB
431 bool ConnStateData::handleReadData() STUB_RETVAL(false)
432 bool ConnStateData::handleRequestBodyData() STUB_RETVAL(false)
433 -void ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, CachePeer *peer, bool auth, bool monitor) STUB
434 +void ConnStateData::pinBusyConnection(const Comm::ConnectionPointer &, const HttpRequest::Pointer &) STUB
435 +void ConnStateData::notePinnedConnectionBecameIdle(PinnedIdleContext) STUB
436 void ConnStateData::unpinConnection(const bool andClose) STUB
437 const Comm::ConnectionPointer ConnStateData::validatePinnedConnection(HttpRequest *request, const CachePeer *peer) STUB_RETVAL(NULL)
438 void ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) STUB
440 void ConnStateData::swanSong() STUB
441 void ConnStateData::quitAfterError(HttpRequest *request) STUB
443 -void ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection) STUB
444 +void ConnStateData::httpsPeeked(PinnedIdleContext) STUB
445 void ConnStateData::getSslContextStart() STUB
446 void ConnStateData::getSslContextDone(SSL_CTX * sslContext, bool isNew) STUB
447 void ConnStateData::sslCrtdHandleReplyWrapper(void *data, const Helper::Reply &reply) STUB