From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Date: Tue, 20 May 2025 18:52:04 +0000 (+0000)
Subject: Bug 5352: Do not get stuck in RESPMOD after pausing peer read(2) (#2065)
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=cfee98c8083f5f5175e72486a6b469e97d9371b0;p=thirdparty%2Fsquid.git

Bug 5352: Do not get stuck in RESPMOD after pausing peer read(2) (#2065)

The transaction gets stuck if Squid, while sending virgin body bytes to
an ICAP RESPMOD service, temporary stops reading additional virgin body
bytes from cache_peer or origin server. Squid pauses reading (with
readSizeWanted becoming zero) if reading more virgin bytes is temporary
prohibited by delay pools and/or read_ahead_gap limits:

    readReply: avoid delayRead() to give adaptation a chance to drain...

HttpStateData::readReply() starts waiting for ModXact to drain the
BodyPipe buffer, but that draining may not happen, either because
ModXact::virginConsume() is not called at all[^1] or because it is
"postponing consumption" when BodyPipe still has some unused space[^2].

With HttpStateData not reading more virgin bytes, Squid may not write
more virgin body bytes to the ICAP service, and the ICAP service may not
start or continue responding to the RESPMOD request. Without that ICAP
activity, ModXact does not consume, the virgin BodyPipe buffer is not
drained, HttpStateData is not reading, and no progress is possible.

HttpStateData::readReply() should start waiting for adaptation to drain
BodyPipe only when the buffer becomes completely full (instead of when
it is not empty). This change may increase virgin response body bytes
accumulation but not the buffer capacity because existing buffer
space-increasing logic in maybeMakeSpaceAvailable() remains intact.

To prevent stalling, both BodyPipe ends (i.e. HttpStateData and
Icap::ModXact) must use matching "progress is possible" conditions, but

* HttpStateData used hasContent()
* Icap::ModXact used spaceSize()
* Ftp::Client used potentialSpaceSize()

Now, all three use matching potentialSpaceSize()-based conditions.

Squid eCAP code is unaffected by this bug, because it does not postpone
BodyPipe consumption. eCAP API does not expose virgin body buffer
capacity, so an eCAP adapter that postpones consumption risks filling
the virgin body buffer and stalling. This is an eCAP API limitation.

Broken since 2024 commit cc8b26f.

[^1]: Zero readSizeWanted is reachable without delay pools, but only if
Squid receives an adapted response (that makes readAheadPolicyCanRead()
false by filling StoreEntry). Ideally, receiving an adapted response
should result in a virginConsume() calls (that would trigger BodyPipe
draining), but currently it may not. Reliably starting virgin data
consumption sooner is not trivial and deserves a dedicated change.

[^2]: ModXact postpones consumption to preserve virgin bytes for ICAP
retries and similar purposes. ModXact believes it is safe to postpone
because there is still space left in the buffer for HttpStateData to
continue to make progress. ModXact would normally start or resume
draining the buffer when sending more virgin bytes to the ICAP service.
---

diff --git a/src/adaptation/icap/ModXact.cc b/src/adaptation/icap/ModXact.cc
index f7201fb8b3..eb8e75eae4 100644
--- a/src/adaptation/icap/ModXact.cc
+++ b/src/adaptation/icap/ModXact.cc
@@ -435,11 +435,10 @@ void Adaptation::Icap::ModXact::virginConsume()
     BodyPipe &bp = *virgin.body_pipe;
     const bool wantToPostpone = isRepeatable || canStartBypass || protectGroupBypass;
 
-    if (wantToPostpone && bp.buf().spaceSize() > 0) {
+    if (wantToPostpone && bp.buf().potentialSpaceSize() > 0) {
         // Postponing may increase memory footprint and slow the HTTP side
         // down. Not postponing may increase the number of ICAP errors
-        // if the ICAP service fails. We may also use "potential" space to
-        // postpone more aggressively. Should the trade-off be configurable?
+        // if the ICAP service fails. Should the trade-off be configurable?
         debugs(93, 8, "postponing consumption from " << bp.status());
         return;
     }
diff --git a/src/http.cc b/src/http.cc
index f15cf5b5ef..c712b4b8bc 100644
--- a/src/http.cc
+++ b/src/http.cc
@@ -1216,7 +1216,7 @@ HttpStateData::readReply(const CommIoCbParams &io)
             return; // wait for Client::noteMoreBodySpaceAvailable()
         }
 
-        if (virginBodyDestination && virginBodyDestination->buf().hasContent()) {
+        if (virginBodyDestination && !virginBodyDestination->buf().hasPotentialSpace()) {
             debugs(11, 5, "avoid delayRead() to give adaptation a chance to drain body pipe buffer: " << virginBodyDestination->buf().contentSize());
             return; // wait for Client::noteMoreBodySpaceAvailable()
         }