Skip to content

Commit 8a1c5cf

Browse files
Do not treat tiny virgin response buffer space specially (#2393)
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. ---- This change is a manual backport of master/v8 commit 76a642b, which is necessary due to a merge conflict created by v7 commit 2669f3c. --------- Co-authored-by: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
1 parent 2e58fa8 commit 8a1c5cf

2 files changed

Lines changed: 2 additions & 2 deletions

File tree

src/clients/FtpClient.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -930,7 +930,7 @@ Ftp::Client::maybeReadVirginBody()
930930

931931
debugs(9, 9, "FTP may read up to " << read_sz << " bytes");
932932

933-
if (read_sz < 2) // see http.cc
933+
if (!read_sz)
934934
return;
935935

936936
data.read_pending = true;

src/http.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1699,7 +1699,7 @@ HttpStateData::maybeMakeSpaceAvailable(const size_t maxReadSize)
16991699
// how much we want to read
17001700
const size_t read_size = calcBufferSpaceToReserve(inBuf.spaceSize(), maxReadSize);
17011701

1702-
if (read_size < 2) {
1702+
if (!read_size) {
17031703
debugs(11, 7, "will not read up to " << read_size << " into buffer (" << inBuf.length() << "/" << inBuf.spaceSize() << ") from " << serverConnection);
17041704
return 0;
17051705
}

0 commit comments

Comments
 (0)