[squid-dev] [PATCH] Fix ICAP transactions that read a lot of data

[Amos Jeffries]

On 25/07/2015 10:51 a.m., Alex Rousskov wrote:
> Hello,
> 
>     Trunk r13995 (Parser-NG: Convert the ICAP read buffer to an SBuf)
> broke ICAP transactions that read a lot of data because the new
> SBuf::consume() method often does not free buffer space, unlike the old
> MemBuf::consume(). Trunk r14173 (Parser-NG: Transfer-Encoding:chunked
> Parser) did not have a material effect on this bug AFAICT, even though
> it replaced at least one new explicit consume() call with an SBuf
> assignment. I think that just effectively moved the consume() call(s)
> elsewhere.
> 
> Affected transactions fail with mayReadMore() exceptions because their
> readBuf.spaceSize() is zero while they need to read more data.
> 
> Any append,parse,consume;append,parse,consume;... user of SBuf cannot
> rely on SBuf::spaceSize() to be meaningful because even consuming the
> entire SBuf contents may leave spaceSize() at zero! Instead such code
> has to use SBuf::length() to keep buffer from growing too big and
> SBuf::rawSpace(1) to ensure some space is available for reading when the
> buffer is not too big.

If you were using rawSpace(1) to avoid the possible reallocation by
reserveSpace() it was not worth it. The reserveCapacity() used above it
is what reserveSpace() uses internally and causes that reallocation. So
its happening anyway.

It is probably better to do:
   rawSpace(SQUID_TCP_SO_RCVBUF - readBuf.length());

Which guarantees the requested size, without triggering as many
reallocates as reserveCapacity().

> 
> The attached patch fixes this bug in code based on trunk r14083. Judging
> by cache.log, the same patch has the right effect on the current trunk
> (r14173) as well. However, the current trunk seems to be broken in other
> (non-ICAP) places [related to large transaction handling?] so I am
> unable to fully test the patch there right now.

I've checked the other client_side.cc/http.cc uses of Comm::ReadNow
which should be roughly equivalent to this part of ICAP and make use of
spaceSize() in buffer growth.

They use length() to calculate size, and spaceSize() to determine if the
grow() calculations are necessary. That was left out of the ICAP logic
on the assumption that ICAP peristent connections would operate better
on full SQUID_TCP_SO_RCVBUF sized buffers instead of small dynamically
grown ones.


client_side.cc/http.cc both also use reserveSpace(N) instead of
rawSpace(N). Which may reallocate a lot more. Seems like a mistake to
avoid the SBuf API marked "EXTREME caution". At least for I/O.

So lots more reading into many smaller buffers. That was one of the
goals right ? :-P

But I'm not sure *how much* smaller than 64K buffers was optimal. We may
be going too far down now on server connections.

> 
> I have not carefully checked whether other new SBuf::consume() users are
> affected. If somebody has the time, I recommend analyzing all
> SBuf::spaceSize() uses.


Checked spaceSize(). Outside the client_side.cc and http.cc calculations
for I/O buffers it is actually the old MemBuf::spaceSize() or just
buffer size display. Nothing broken.

Not sure exactly what to look for on consume() uses. It will be all
Parser in Squid though. And where read buffer feed into BodyPipe.


Anyhow:
  +1, Please apply ASAP after considering the above mentioned alteration
of rawSpace(1).

Amos