From: Eduard Bagdasaryan Date: Mon, 11 Jun 2018 10:44:37 +0000 (+0000) Subject: Bug 4223: fixed retries of failed re-forwardable transactions (#211) X-Git-Tag: M-staged-PR221~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=70eb3fde5e3bbfbab7ce310393b32ce785ab2881;p=thirdparty%2Fsquid.git 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 146dbb53dc..f692a114a7 100644 --- 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 15c7159423..732451a77c 100644 --- 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 5ca9180af9..52953f22cd 100644 --- 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 620e68180a..19da1cfb14 100644 --- 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 bd4411776a..b686d5c0d0 100644 --- 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 c3bff07f11..646e6b403b 100644 --- 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 d5fd18a8aa..5789613b9c 100644 --- 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 a44a8ef23f..f9760a4037 100644 --- 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 e5aff1ba2b..372b7c3a96 100644 --- 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 7041a7213c..d41fedcffa 100644 --- 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(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 c0277c4422..a7098019f5 100644 --- 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 b825401110..b13c561572 100644 --- 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;