Bug 4223: fixed retries of failed re-forwardable transactions (#211)

This change fixes Store to delay writing (and, therefore, sharing) of
re-triable errors (e.g., 504 Gateway Timeout responses) to clients:
once we start sharing the response with a client, we cannot re-try the
transaction. Since ENTRY_FWD_HDR_WAIT flag purpose is to delay response
sharing with Store clients, this patch fixes and clarifies its usage:

1. Removes unconditional clearing from startWriting().
2. Adds a conditional clearing to StoreEntry::write().
3. Sets it only for may-be-rejected responses.

(2) adds ENTRY_FWD_HDR_WAIT clearing to detect responses that filled the
entire read buffer and must be shared now with the clients because
they can no longer be retried with the current re-forwarding mechanisms
(which rely on completing the bad response transaction first) and will
get stuck. Such large re-triable error responses (>16KB with default
read_ahead_gap) should be uncommon. They were getting stuck prior to
master r12501.1.48. That revision started clearing ENTRY_FWD_HDR_WAIT in
StoreEntry startWriting() unconditionally, allowing all errors to be
sent to Store clients without a delay, and effectively disabling
retries.

Mgr::Forwarder was always setting ENTRY_FWD_HDR_WAIT, probably mimicking
similarly aggressive FwdState behavior that we are now removing. Since
the forwarder never rewrites the entry content, it should not need to
set that flag. The forwarder and associated Server classes must not
communicate with the mgr client during the client-Squid connection
descriptor handoff to Coordinator, but ENTRY_FWD_HDR_WAIT is not the
right mechanism to block such Squid-client communication. The flag
does not work well for this purpose because those Server classes may
(and do?) substitute the "blocked" StoreEntry with another one to
write an error message to the client.

Also moved ENTRY_FWD_HDR_WAIT clearance from many StoreEntry::complete()
callers to that method itself. StoreEntry::complete() is meant to be the
last call when forming the entry. Any post-complete entry modifications
such as retries are prohibited.

diff --git a/src/FwdState.cc b/src/FwdState.cc
index ec1414b8c2411e803bff709ba3023bacbd275f27..f2013333f8c7d114c29609b1b3cc3c243c9f8b22 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -143,7 +143,6 @@ FwdState::FwdState(const Comm::ConnectionPointer &client, StoreEntry * e, HttpRe
     HTTPMSGLOCK(request);
     serverDestinations.reserve(Config.forward_max_tries);
     e->lock("FwdState");
-    EBIT_SET(e->flags, ENTRY_FWD_HDR_WAIT);
     flags.connected_okay = false;
     flags.dont_retry = false;
     flags.forward_completed = false;
@@ -258,7 +257,6 @@ FwdState::completed()
             }
 #endif
         } else {
-            EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
             entry->complete();
             entry->releaseRequest();
         }
@@ -528,7 +526,6 @@ FwdState::complete()
             debugs(17, 3, HERE << "server FD " << serverConnection()->fd << " not re-forwarding status " << entry->getReply()->sline.status());
         else
             debugs(17, 3, HERE << "server (FD closed) not re-forwarding status " << entry->getReply()->sline.status());
-        EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
         entry->complete();
 
         if (!Comm::IsConnOpen(serverConn))

diff --git a/src/MemStore.cc b/src/MemStore.cc
index 1f5df4d8992c6b0ab2196a24a6531976526b6058..e0ebe3862872c06818d7d5438760fdefd62e24db 100644 (file)
--- a/src/MemStore.cc
+++ b/src/MemStore.cc
@@ -556,7 +556,6 @@ MemStore::copyFromShmSlice(StoreEntry &e, const StoreIOBuffer &buf, bool eof)
         const int result = rep->httpMsgParseStep(mb.buf, buf.length, eof);
         if (result > 0) {
             assert(rep->pstate == psParsed);
-            EBIT_CLR(e.flags, ENTRY_FWD_HDR_WAIT);
         } else if (result < 0) {
             debugs(20, DBG_IMPORTANT, "Corrupted mem-cached headers: " << e);
             return false;
@@ -657,15 +656,9 @@ MemStore::startCaching(StoreEntry &e)
 void
 MemStore::copyToShm(StoreEntry &e)
 {
-    // prevents remote readers from getting ENTRY_FWD_HDR_WAIT entries and
-    // not knowing when the wait is over
-    if (EBIT_TEST(e.flags, ENTRY_FWD_HDR_WAIT)) {
-        debugs(20, 5, "postponing copying " << e << " for ENTRY_FWD_HDR_WAIT");
-        return;
-    }
-
     assert(map);
     assert(e.mem_obj);
+    Must(!EBIT_TEST(e.flags, ENTRY_FWD_HDR_WAIT));
 
     const int64_t eSize = e.mem_obj->endOffset();
     if (e.mem_obj->memCache.offset >= eSize) {

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index ef876c295d378d6a550e930b11d8cd6ef5809ae7..d6e73b4d9042be1478bb94728c8210dcc80294ad 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1953,7 +1953,6 @@ ClientHttpRequest::handleAdaptedHeader(HttpMsg *msg)
         assert(repContext);
         repContext->createStoreEntry(request->method, request->flags);
 
-        EBIT_CLR(storeEntry()->flags, ENTRY_FWD_HDR_WAIT);
         request_satisfaction_mode = true;
         request_satisfaction_offset = 0;
         storeEntry()->replaceHttpReply(new_rep);

diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc
index d0bfb8014987089a72e4ccaeb88a6b371fc98584..41331ccef6f36cb6c59cbd724201c7c5cf093425 100644 (file)
--- a/src/clients/FtpGateway.cc
+++ b/src/clients/FtpGateway.cc
@@ -2289,8 +2289,7 @@ Ftp::Gateway::completedListing()
     ferr.ftp.cwd_msg = xstrdup(cwd_message.size()? cwd_message.termedBuf() : "");
     ferr.ftp.server_msg = ctrl.message;
     ctrl.message = NULL;
-    entry->replaceHttpReply( ferr.BuildHttpReply() );
-    EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
+    entry->replaceHttpReply(ferr.BuildHttpReply());
     entry->flush();
     entry->unlock("Ftp::Gateway");
 }
@@ -2563,8 +2562,6 @@ Ftp::Gateway::appendSuccessHeader()
 
     assert(entry->isEmpty());
 
-    EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
-
     entry->buffer();    /* released when done processing current data payload */
 
     SBuf urlPath = request->url.path();

diff --git a/src/clients/FtpRelay.cc b/src/clients/FtpRelay.cc
index a63b3282f0b549376216e31a917cc20b722bb955..d3755d07debaaec60a477d37ff26a331e78921ab 100644 (file)
--- a/src/clients/FtpRelay.cc
+++ b/src/clients/FtpRelay.cc
@@ -292,7 +292,6 @@ Ftp::Relay::failedErrorMessage(err_type error, int xerrno)
     const Http::StatusCode httpStatus = failedHttpStatus(error);
     HttpReply *const reply = createHttpReply(httpStatus);
     entry->replaceHttpReply(reply);
-    EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
     fwd->request->detailError(error, xerrno);
 }
 
@@ -375,7 +374,6 @@ void
 Ftp::Relay::forwardReply()
 {
     assert(entry->isEmpty());
-    EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
 
     HttpReply *const reply = createHttpReply(Http::scNoContent);
     reply->sources |= HttpMsg::srcFtp;
@@ -453,7 +451,6 @@ Ftp::Relay::startDataDownload()
     HttpReply *const reply = createHttpReply(Http::scOkay, -1);
     reply->sources |= HttpMsg::srcFtp;
 
-    EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
     setVirginReply(reply);
     adaptOrFinalizeReply();
 

diff --git a/src/enums.h b/src/enums.h
index 8e8f86c2cc003ab1c417587e31ddbf0609a892d2..63eda6b7a31d6b6dad1c42bf0857b2511af9423d 100644 (file)
--- a/src/enums.h
+++ b/src/enums.h
@@ -75,12 +75,31 @@ typedef enum {
 enum {
     ENTRY_SPECIAL,
     ENTRY_REVALIDATE_ALWAYS,
+
+    /// Tiny Store writes are likely. The writes should be aggregated together
+    /// before Squid announces the new content availability to the store
+    /// clients. For example, forming a cached HTTP response header may result
+    /// in dozens of StoreEntry::write() calls, many of which adding as little
+    /// as two bytes. Sharing those small writes with the store clients
+    /// increases overhead, especially because the client code can do nothing
+    /// useful with the written content until the whole response header is
+    /// stored. Might be combined with ENTRY_FWD_HDR_WAIT. TODO: Rename to
+    /// ENTRY_DELAY_WHILE_COALESCING to emphasize the difference from and
+    /// similarity with ENTRY_FWD_HDR_WAIT.
     DELAY_SENDING,
     RELEASE_REQUEST, ///< prohibits making the key public
     REFRESH_REQUEST,
     ENTRY_REVALIDATE_STALE,
     ENTRY_DISPATCHED,
     KEY_PRIVATE,
+
+    /// The current entry response may change. The contents of an entry in this
+    /// state must not be shared with its store clients. For example, Squid
+    /// receives (and buffers) an HTTP/504 response but may decide to retry that
+    /// transaction to receive a successful response from another server
+    /// instead. Might be combined with DELAY_SENDING. TODO: Rename to
+    /// ENTRY_DELAY_WHILE_WOBBLING to emphasize the difference from and
+    /// similarity with DELAY_SENDING.
     ENTRY_FWD_HDR_WAIT,
     ENTRY_NEGCACHED,
     ENTRY_VALIDATED,

diff --git a/src/gopher.cc b/src/gopher.cc
index 6aec0c156db9498123a575aee9ece5e375a06cea..aaabb93c43418fad0d38d826e94989ea8097ba22 100644 (file)
--- a/src/gopher.cc
+++ b/src/gopher.cc
@@ -241,7 +241,6 @@ gopherMimeCreate(GopherStateData * gopherState)
     }
 
     assert(entry->isEmpty());
-    EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
 
     HttpReply *reply = new HttpReply;
     entry->buffer();

diff --git a/src/http.cc b/src/http.cc
index 42fd7a1cfa60103cef51b0a0a2062f95b33aaa53..6d60b2051a89e062a479b94c61f454a72ef4e793 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -928,8 +928,8 @@ HttpStateData::haveParsedReplyHeaders()
             // TODO: check whether such responses are shareable.
             // Do not share for now.
             entry->makePrivate(false);
-            if (!fwd->reforwardableStatus(rep->sline.status()))
-                EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
+            if (fwd->reforwardableStatus(rep->sline.status()))
+                EBIT_SET(entry->flags, ENTRY_FWD_HDR_WAIT);
             varyFailure = true;
         } else {
             entry->mem_obj->vary_headers = vary;
@@ -947,8 +947,8 @@ HttpStateData::haveParsedReplyHeaders()
          * If its not a reply that we will re-forward, then
          * allow the client to get it.
          */
-        if (!fwd->reforwardableStatus(rep->sline.status()))
-            EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
+        if (fwd->reforwardableStatus(rep->sline.status()))
+            EBIT_SET(entry->flags, ENTRY_FWD_HDR_WAIT);
 
         ReuseDecision decision(entry, statusCode);
 

diff --git a/src/ipc/Forwarder.cc b/src/ipc/Forwarder.cc
index e5aff1ba2b942c647fd08d762424059b57dcec51..372b7c3a96b39a1071e000d7e5acfe3da5e27797 100644 (file)
--- a/src/ipc/Forwarder.cc
+++ b/src/ipc/Forwarder.cc
@@ -87,8 +87,10 @@ Ipc::Forwarder::handleRemoteAck()
 {
     debugs(54, 3, HERE);
     request->requestId = 0;
-    // Do not clear ENTRY_FWD_HDR_WAIT or do entry->complete() because
-    // it will trigger our client side processing. Let job cleanup close.
+    // Do not do entry->complete() because it will trigger our client side
+    // processing when we no longer own the client-Squid connection.
+    // Let job cleanup close the client-Squid connection that Coordinator
+    // now owns.
 }
 
 /// Ipc::Forwarder::requestTimedOut wrapper

diff --git a/src/mgr/Forwarder.cc b/src/mgr/Forwarder.cc
index 7041a7213c81a12547e4ab8b089064a6174480b6..d41fedcffa891ed709ead6c9efc4520b776e00db 100644 (file)
--- a/src/mgr/Forwarder.cc
+++ b/src/mgr/Forwarder.cc
@@ -37,7 +37,6 @@ Mgr::Forwarder::Forwarder(const Comm::ConnectionPointer &aConn, const ActionPara
 
     HTTPMSGLOCK(httpRequest);
     entry->lock("Mgr::Forwarder");
-    EBIT_SET(entry->flags, ENTRY_FWD_HDR_WAIT);
 
     closer = asyncCall(16, 5, "Mgr::Forwarder::noteCommClosed",
                        CommCbMemFunT<Forwarder, CommCloseCbParams>(this, &Forwarder::noteCommClosed));
@@ -110,7 +109,6 @@ Mgr::Forwarder::sendError(ErrorState *error)
     Must(entry != NULL);
     Must(httpRequest != NULL);
 
-    EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
     entry->buffer();
     entry->replaceHttpReply(error->BuildHttpReply());
     entry->expires = squid_curtime;

diff --git a/src/store.cc b/src/store.cc
index 6595f5bc94df263c2b91d47753238252f29845fc..aa39cb4152c81cfc59112d1c4ddbeb77ac2ae3a7 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -828,8 +828,12 @@ StoreEntry::write (StoreIOBuffer writeBuffer)
     storeGetMemSpace(writeBuffer.length);
     mem_obj->write(writeBuffer);
 
-    if (!EBIT_TEST(flags, DELAY_SENDING))
-        invokeHandlers();
+    if (EBIT_TEST(flags, ENTRY_FWD_HDR_WAIT) && !mem_obj->readAheadPolicyCanRead()) {
+        debugs(20, 3, "allow Store clients to get entry content after buffering too much for " << *this);
+        EBIT_CLR(flags, ENTRY_FWD_HDR_WAIT);
+    }
+
+    invokeHandlers();
 }
 
 /* Append incoming data from a primary server to an entry. */
@@ -1073,6 +1077,9 @@ StoreEntry::complete()
 {
     debugs(20, 3, "storeComplete: '" << getMD5Text() << "'");
 
+    // To preserve forwarding retries, call FwdState::complete() instead.
+    EBIT_CLR(flags, ENTRY_FWD_HDR_WAIT);
+
     if (store_status != STORE_PENDING) {
         /*
          * if we're not STORE_PENDING, then probably we got aborted
@@ -1129,6 +1136,9 @@ StoreEntry::abort()
 
     EBIT_SET(flags, ENTRY_ABORTED);
 
+    // allow the Store clients to be told about the problem
+    EBIT_CLR(flags, ENTRY_FWD_HDR_WAIT);
+
     setMemStatus(NOT_IN_MEMORY);
 
     store_status = STORE_OK;
@@ -1823,7 +1833,6 @@ StoreEntry::startWriting()
     buffer();
     rep->packHeadersInto(this);
     mem_obj->markEndOfReplyHeaders();
-    EBIT_CLR(flags, ENTRY_FWD_HDR_WAIT);
 
     rep->body.packInto(this);
     flush();

diff --git a/src/store_client.cc b/src/store_client.cc
index 7e4f21dc5a6b9728ffd5243019e6887bdfc475df..06a278ca603ca41d68edc4bac28f3caad1fb3376 100644 (file)
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -279,11 +279,6 @@ storeClientCopy2(StoreEntry * e, store_client * sc)
         return;
     }
 
-    if (EBIT_TEST(e->flags, ENTRY_FWD_HDR_WAIT)) {
-        debugs(90, 5, "storeClientCopy2: returning because ENTRY_FWD_HDR_WAIT set");
-        return;
-    }
-
     if (sc->flags.store_copying) {
         sc->flags.copy_event_pending = true;
         debugs(90, 3, "storeClientCopy2: Queueing storeClientCopyEvent()");
@@ -711,6 +706,15 @@ storeUnregister(store_client * sc, StoreEntry * e, void *data)
 void
 StoreEntry::invokeHandlers()
 {
+    if (EBIT_TEST(flags, DELAY_SENDING)) {
+        debugs(90, 3, "DELAY_SENDING is on, exiting " << *this);
+        return;
+    }
+    if (EBIT_TEST(flags, ENTRY_FWD_HDR_WAIT)) {
+        debugs(90, 3, "ENTRY_FWD_HDR_WAIT is on, exiting " << *this);
+        return;
+    }
+
     /* Commit what we can to disk, if appropriate */
     swapOut();
     int i = 0;