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. 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 have not carefully checked whether other new SBuf::consume() users are affected. If somebody has the time, I recommend analyzing all SBuf::spaceSize() uses. Thank you, Alex.
Fix ICAP transactions that read a lot of data by ensuring the read buffer has space [unless it is really full]. 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(). Affected transactions failed with mayReadMore() exceptions because their readBuf.spaceSize() was zero while they needed 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. === modified file 'src/adaptation/icap/ModXact.cc' --- src/adaptation/icap/ModXact.cc 2015-01-25 15:05:23 +0000 +++ src/adaptation/icap/ModXact.cc 2015-07-24 21:16:32 +0000 @@ -540,44 +540,44 @@ void Adaptation::Icap::ModXact::startRea Must(!adapted.body_pipe); // we use the same buffer for headers and body and then consume headers readMore(); } void Adaptation::Icap::ModXact::readMore() { if (reader != NULL || doneReading()) { debugs(93,3,HERE << "returning from readMore because reader or doneReading()"); return; } // do not fill readBuf if we have no space to store the result if (adapted.body_pipe != NULL && !adapted.body_pipe->buf().hasPotentialSpace()) { debugs(93,3,HERE << "not reading because ICAP reply pipe is full"); return; } - if (readBuf.spaceSize()) + if (readBuf.length() < SQUID_TCP_SO_RCVBUF) scheduleRead(); else - debugs(93,3,HERE << "nothing to do because !readBuf.spaceSize()"); + debugs(93,3,HERE << "cannot read with a full buffer"); } // comm module read a portion of the ICAP response for us void Adaptation::Icap::ModXact::handleCommRead(size_t) { Must(!state.doneParsing()); icap_tio_finish = current_time; parseMore(); readMore(); } void Adaptation::Icap::ModXact::echoMore() { Must(state.sending == State::sendingVirgin); Must(adapted.body_pipe != NULL); Must(virginBodySending.active()); const size_t sizeMax = virginContentSize(virginBodySending); debugs(93,5, HERE << "will echo up to " << sizeMax << " bytes from " << virgin.body_pipe->status()); === modified file 'src/adaptation/icap/Xaction.cc' --- src/adaptation/icap/Xaction.cc 2015-06-02 21:35:51 +0000 +++ src/adaptation/icap/Xaction.cc 2015-07-24 21:29:25 +0000 @@ -439,40 +439,45 @@ void Adaptation::Icap::Xaction::schedule Must(haveConnection()); Must(!reader); Must(readBuf.length() < SQUID_TCP_SO_RCVBUF); // will expand later if needed typedef CommCbMemFunT<Adaptation::Icap::Xaction, CommIoCbParams> Dialer; reader = JobCallback(93, 3, Dialer, this, Adaptation::Icap::Xaction::noteCommRead); Comm::Read(connection, reader); updateTimeout(); } // comm module read a portion of the ICAP response for us void Adaptation::Icap::Xaction::noteCommRead(const CommIoCbParams &io) { Must(reader != NULL); reader = NULL; Must(io.flag == Comm::OK); // TODO: tune this better to expected message sizes readBuf.reserveCapacity(SQUID_TCP_SO_RCVBUF); + // we are not asked to grow beyond the allowed maximum + Must(readBuf.length() < SQUID_TCP_SO_RCVBUF); + // now we can ensure that there is space to read new data, + // even if readBuf.spaceSize() currently returns zero. + readBuf.rawSpace(1); CommIoCbParams rd(this); // will be expanded with ReadNow results rd.conn = io.conn; switch (Comm::ReadNow(rd, readBuf)) { case Comm::INPROGRESS: if (readBuf.isEmpty()) debugs(33, 2, io.conn << ": no data to process, " << xstrerr(rd.xerrno)); scheduleRead(); return; case Comm::OK: al.icap.bytesRead += rd.size; updateTimeout(); debugs(93, 3, "read " << rd.size << " bytes"); disableRetries(); // because pconn did not fail @@ -517,41 +522,41 @@ bool Adaptation::Icap::Xaction::parseHtt Http::StatusCode error = Http::scNone; // XXX: performance regression c_str() data copies const char *buf = readBuf.c_str(); const bool parsed = msg->parse(buf, readBuf.length(), commEof, &error); Must(parsed || !error); // success or need more data if (!parsed) { // need more data Must(mayReadMore()); msg->reset(); return false; } readBuf.consume(msg->hdr_sz); return true; } bool Adaptation::Icap::Xaction::mayReadMore() const { return !doneReading() && // will read more data - readBuf.spaceSize(); // have space for more data + readBuf.length() < SQUID_TCP_SO_RCVBUF; // have space for more data } bool Adaptation::Icap::Xaction::doneReading() const { return commEof; } bool Adaptation::Icap::Xaction::doneWriting() const { return !writer; } bool Adaptation::Icap::Xaction::doneWithIo() const { return haveConnection() && !connector && !reader && !writer && // fast checks, some redundant doneReading() && doneWriting(); } bool Adaptation::Icap::Xaction::haveConnection() const
_______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev