Do not treat tiny virgin response buffer space specially#2393
Open
rousskov wants to merge 3 commits intosquid-cache:v7from
Open
Do not treat tiny virgin response buffer space specially#2393rousskov wants to merge 3 commits intosquid-cache:v7from
rousskov wants to merge 3 commits intosquid-cache:v7from
Conversation
…(2)" This reverts commit 2669f3c to apply an earlier master/v8 change first.
A long series of commits sprinkled Squid code with special treatment of virgin response buffer space equal to one or two bytes: * 2005 commit 2afaba0 adds a 2-byte hack to HttpStateData. The hack avoids stalling transactions by never asking StoreEntry::delayAwareRead() to read 1 byte. * 2006 commit 253cacc adds the same 2-byte hack to Ftp::Client. * 2007 commit 478cfe9 adds a 2-byte hack to Icap::ModXact::virginConsume() so that ICAP transactions do not stall when HttpStateData hack prevents new virgin bytes from being read into a BodyPipe buffer that has just one or two space bytes left. The ICAP hack was probably one-byte more aggressive than necessary... * 2011 commit 0ad2b63 adds +1 byte hacks to ServerStateData and ClientHttpRequest to work around the same StoreEntry::delayAwareRead() problem leading to excessive buffering with slow clients. * A followup 2012 commit 4dc2b07 adjusts StoreEntry::delayAwareRead() to allow 1-byte reads, making _all_ of the above hacks unnecessary. However, that commit only removed +1 byte hacks added in 2011, missing HttpStateData and ICAP hacks added in 2005, 2006 and 2007, probably because that 2012 work focused on fixing the bug introduced by 2011 commit (that was triggered by slow client-Squid communication). We now remove the remaining known hacks that worked around StoreEntry::delayAwareRead() inability to read 1 byte (i.e. the first three hacks on the above list). These changes also happen to fix transaction stalls with read_ahead_gap set to 1 and probably some other rare use cases: Transactions could stall because inBuf.spaceSize() may be 1 for an emptied inBuf, leading to maybeMakeSpaceAvailable() returning zero due to `read_size < 2` being true. That zero result prevents HttpStateData from making progress by reading new virgin response bytes. Mistreatment of "emptied inBuf" cases was exposed by 2023 commit 50c5af8 changes. More work is needed to address other, more prominent "emptied inBuf" cases. ---- Cherry-picked master/v8 commit 76a642b.
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.
----
Cherry-picked master/v8 commit cfee98c
and effectively restored changes reverted two commits ago.
Contributor
Author
|
When we wrote master/v8 76a642b (i.e. #2056), we were not sure whether it fixes any "real" bugs. See that commit message (copied to this backporting PR description) for details. We now know that it does fix a bug recently described on squid-users: Log analysis suggests and initial v7-based deployments confirm that the changes in this PR fix that bug.
This PR branch "revert v7 2669f3c backport of master/v8 cfee98c, apply master/v8 76a642b, reapply master/v8 cfee98c" commit sequence demonstrates that the merge conflict can be avoided by changing cherry-picking order to match the order of master/v8 commits. |
rousskov
added
the
S-waiting-for-maintainer
kinkie
approved these changes
Mar 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A long series of commits sprinkled Squid code with special treatment of
virgin response buffer space equal to one or two bytes:
2005 commit 2afaba0 adds a 2-byte hack to HttpStateData. The hack
avoids stalling transactions by never asking
StoreEntry::delayAwareRead() to read 1 byte.
2006 commit 253cacc adds the same 2-byte hack to Ftp::Client.
2007 commit 478cfe9 adds a 2-byte hack to
Icap::ModXact::virginConsume() so that ICAP transactions do not stall
when HttpStateData hack prevents new virgin bytes from being read into
a BodyPipe buffer that has just one or two space bytes left. The ICAP
hack was probably one-byte more aggressive than necessary...
2011 commit 0ad2b63 adds +1 byte hacks to ServerStateData and
ClientHttpRequest to work around the same StoreEntry::delayAwareRead()
problem leading to excessive buffering with slow clients.
A followup 2012 commit 4dc2b07 adjusts StoreEntry::delayAwareRead()
to allow 1-byte reads, making all of the above hacks unnecessary.
However, that commit only removed +1 byte hacks added in 2011, missing
HttpStateData and ICAP hacks added in 2005, 2006 and 2007, probably
because that 2012 work focused on fixing the bug introduced by 2011
commit (that was triggered by slow client-Squid communication).
We now remove the remaining known hacks that worked around
StoreEntry::delayAwareRead() inability to read 1 byte (i.e. the first
three hacks on the above list).
These changes also happen to fix transaction stalls with read_ahead_gap
set to 1 and probably some other rare use cases: Transactions could
stall because inBuf.spaceSize() may be 1 for an emptied inBuf, leading
to maybeMakeSpaceAvailable() returning zero due to
read_size < 2beingtrue. That zero result prevents HttpStateData from making progress by
reading new virgin response bytes. Mistreatment of "emptied inBuf" cases
was exposed by 2023 commit 50c5af8 changes. More work is needed to
address other, more prominent "emptied inBuf" cases.
This change is a manual backport of master/v8 commit 76a642b, which is
necessary due to a merge conflict created by v7 commit 2669f3c.