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 146dbb53dcd7b143423f1ebb6aba130ec25e7554..f692a114a7856e3402c09026912e225600ff5b9e 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -142,7 +142,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;
@@ -267,7 +266,6 @@ FwdState::completed()
             }
 #endif
         } else {
-            EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
             entry->complete();
             entry->releaseRequest();
         }
@@ -541,7 +539,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 15c71594237a3f6f82b68e60b91411fec23b0879..732451a77cd41550032bb02570ec3dde7475ef12 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 == Http::Message::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 5ca9180af92ae41be0c313d3ea8923344b486730..52953f22cd08b908170caca83e5adf451005b5bc 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1941,7 +1941,6 @@ ClientHttpRequest::handleAdaptedHeader(Http::Message *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 620e68180a0db3e0d7f9c5b0706e25c4eb403931..19da1cfb14068b00d20e9919538c47b3eecbab0e 100644 (file)
--- a/src/clients/FtpGateway.cc
+++ b/src/clients/FtpGateway.cc
@@ -2289,7 +2289,6 @@ Ftp::Gateway::completedListing()
     ferr.ftp.server_msg = ctrl.message;
     ctrl.message = NULL;
     entry->replaceHttpReply(ferr.BuildHttpReply());
-    EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
     entry->flush();
     entry->unlock("Ftp::Gateway");
 }
@@ -2562,8 +2561,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 bd4411776aade2a2ba8d3be00f0878c4e344d49d..b686d5c0d07f6cf8b590c72fe76317ab41e7975d 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 |= Http::Message::srcFtp;
@@ -453,7 +451,6 @@ Ftp::Relay::startDataDownload()
     HttpReply *const reply = createHttpReply(Http::scOkay, -1);
     reply->sources |= Http::Message::srcFtp;
 
-    EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
     setVirginReply(reply);
     adaptOrFinalizeReply();
 

diff --git a/src/enums.h b/src/enums.h
index c3bff07f11f292e8298534c9c2849cd8053a3658..646e6b403b56cc20fdc85bf81dfb9940449a48b7 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 d5fd18a8aa7caf36f3025dc116778a8218c3b80f..5789613b9cc6eb10981c70393c46d629d5ee9b6c 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 a44a8ef23f883bba5dccda494d24588b4ca26258..f9760a4037a90eb3867642ffe0d478997747d856 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 c0277c44226b3c8f6032522d2957c7422d4c1327..a7098019f57923ac2103ee4243ecad7357a7ae2e 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;
@@ -1818,7 +1828,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 b825401110214cddeec0fccaf25734c2737c52b2..b13c561572a8cde94b835023df90fb67364a180a 100644 (file)
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -312,11 +312,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()");
@@ -744,6 +739,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;