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 _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
