[+ squid-dev; bcc ssquid-users]
Hi Alex,
Sorry about the late reply.
Please see inline.
>> Here's the behavior I have seen: When the connection is set up, the
>> buffer gets a size of 16KB (default). Squid reads from the socket,
>> parses the data, and then sends it towards c-icap as appropriate. Now,
>> as part of parsing the data, the buffer is NUL-terminated via a call
>> to c_str(). This NUL-termination, however, is not accounted for by an
>> increase in the "offset" (off) in the underlying MemBlob, therefore,
>> the offset and size go out of sync.
>
> Just to avoid a misunderstanding:
>
> * MemBlob does not have an "offset".
Indeed. I was imprecise in my explanation -- the effect of drafting
email even as I was in the middle of investigating the code.
The c_str() code doesn't increment SBuf::len_. As a result the
MemBlob::canAppend() call, which takes in SBuf::off_ and SBuf::len_
doesn't match the MemBlob::size_, which was incremented as part of the
c_str() call.
> * A call to c_str() should not increase SBuf::len_ either because it
> does not add a new character to the SBuf object. That call just
> terminates the underlying buffer.
Well, without an increment of the MemBlob::size_ (or with an increment
to the SBuf::len_) this would have been OK. However, once that 'size_'
is incremented, we have the SBuf concept of how much buffer is used
being "out of sync" with the MemBlob's conception of buffer usage.
FWIW I don't quite understand how the NUL-char doesn't add a new
character to the SBuf object. Yes, it is terminating the string, but
in 'C' it is also a legit character. So, unclear what we are
attempting with this magic; or why. Seems to me, not incrementing
'len_' is a mistake.
> Single-owner optimizations aside (a known TODO), the above is the
> desired behavior according to the documented c_str() guarantees:
Can you please explain or point me to a document that has more info
about this "Single-owner" optimization?
>
>> * The returned value points to an internal location whose contents
>> * are guaranteed to remain unchanged only until the next call
>> * to a non-constant member function of the SBuf object.
>
> In other words, we cannot allow some _other_ SBuf object to overwrite
> our null-termination character in the MemBlob we share with that other SBuf.
>
> The high price for that strong guarantee is one of the reasons we should
> avoid c_str() calls in Squid code.
Note that the issue that I have explained is from a mostly stock squid
3.5.1 codebase. This isn't stemming from new c_str() calls added to
the codebase.
>> When canAppend() fails, a new
>> buffer is re-allocated. When this reallocation occurs, however, the
>> new size of the buffer is dependent on the size being reserved.
>
> If we are still talking about the I/O buffer (and not just some random
> SBuf string somewhere), then the I/O buffer _capacity_ should not shrink
> below a certain minimum, regardless of how much content the buffer has
> already stored. There should be some Squid code that ensures the minimum
> capacity of the I/O buffer used to read requests. If it is missing, it
> is a Squid bug.
It does shrink, as you can see from the debugs that I posted earlier.
>> As a temporary measure, I have an experimental change that checks
>> whether the body size is known and if known always reserves a large
>> enough size (currently 16K).
>
> It is difficult to discuss this without seeing your changes, but the
> reservation should probably be unconditional -- the I/O buffer capacity
> should always be at least 16KB (or whatever size we start with).
Yes, that would be another way of fixing this issue.
I have posted the current changes that are currently working for the
most part below. It uses current capacity size as a heuristic to bump
size up. The diff also has some previous fixes, that were pointed out
to me on the thread, embedded in it.
---
diff --git a/3rdparty/squid-3.5.1/src/MemBlob.h b/3rdparty/squid-3.5.1/src/Mem
Blob.h
index b96330e..d265576 100644
--- a/3rdparty/squid-3.5.1/src/MemBlob.h
+++ b/3rdparty/squid-3.5.1/src/MemBlob.h
@@ -94,6 +94,8 @@ public:
/// extends the available space to the entire allocated blob
void clear() { size = 0; }
+ size_type currentCapacity() const { return (capacity); };
+
/// dump debugging information
std::ostream & dump(std::ostream &os) const;
diff --git a/3rdparty/squid-3.5.1/src/SBuf.cc b/3rdparty/squid-3.5.1/src/SBuf.
cc
index 53221d6..91886a0 100644
--- a/3rdparty/squid-3.5.1/src/SBuf.cc
+++ b/3rdparty/squid-3.5.1/src/SBuf.cc
@@ -76,7 +76,7 @@ SBufStats::operator +=(const SBufStats& ss)
SBuf::SBuf()
: store_(GetStorePrototype()), off_(0), len_(0)
{
- debugs(24, 8, id << " created");
+ debugs(24, 8, id << " created, size=" << spaceSize());
++stats.alloc;
++stats.live;
}
@@ -171,6 +171,7 @@ SBuf::rawSpace(size_type minSpace)
// the store knows the last-used portion. If
// it's available, we're effectively claiming ownership
// of it. If it's not, we need to go away (realloc)
+ debugs(24, 7, "off_=" << off_ << "; len_=" << len_ << "; size="
<< store_->size);
if (store_->canAppend(off_+len_, minSpace)) {
debugs(24, 7, "not growing");
return bufEnd();
diff --git a/3rdparty/squid-3.5.1/src/SBuf.h b/3rdparty/squid-3.5.1/src/SBuf.h
index ef77733..5bb3ef4 100644
--- a/3rdparty/squid-3.5.1/src/SBuf.h
+++ b/3rdparty/squid-3.5.1/src/SBuf.h
@@ -541,6 +541,8 @@ public:
/// std::string export function
std::string toStdString() const { return std::string(buf(),length()); }
+ size_type currentCapacity() const { return (store_->currentCapacity()); }
+
// TODO: possibly implement erase() similar to std::string's erase
// TODO: possibly implement a replace() call
private:
diff --git a/3rdparty/squid-3.5.1/src/client_side.cc
b/3rdparty/squid-3.5.1/src/client_side.cc
index f2d0ce0..e191550 100644
--- a/3rdparty/squid-3.5.1/src/client_side.cc
+++ b/3rdparty/squid-3.5.1/src/client_side.cc
@@ -2347,6 +2348,9 @@ ConnStateData::In::maybeMakeSpaceAvailable()
buf.reserveCapacity(wantCapacity);
}
debugs(33, 2, "growing request buffer: available=" <<
buf.spaceSize() << " used=" << buf.length());
+ } else if (buf.spaceSize() < buf.currentCapacity() / 2) {
+ debugs(33, 2, "growing request buffer: available=" <<
buf.spaceSize() << " used=" << buf.length());
+ buf.reserveCapacity(CLIENT_REQ_BUF_SZ * 4);
}
return (buf.spaceSize() >= 2);
}
@@ -3244,6 +3248,7 @@ ConnStateData::clientReadRequest(const CommIoCbParams &io)
* Plus, it breaks our lame *HalfClosed() detection
*/
+ in.maybeMakeSpaceAvailable();
CommIoCbParams rd(this); // will be expanded with ReadNow results
rd.conn = io.conn;
switch (Comm::ReadNow(rd, in.buf)) {
@@ -3354,11 +3359,13 @@ ConnStateData::handleRequestBodyData()
return false;
}
} else { // identity encoding
- debugs(33,5, HERE << "handling plain request body for " <<
clientConnection);
- putSize = bodyPipe->putMoreData(in.buf.c_str(), in.buf.length());
- if (!bodyPipe->mayNeedMoreData()) {
- // BodyPipe will clear us automagically when we produced everything
- bodyPipe = NULL;
+ debugs(33,5, HERE << "handling plain request body for " <<
clientConnection << "; len=" << in.buf.length());
+ if (in.buf.length() > 0) {
+ putSize = bodyPipe->putMoreData(in.buf.c_str(), in.buf.length());
+ if (!bodyPipe->mayNeedMoreData()) {
+ // BodyPipe will clear us automagically when we
produced everything
+ bodyPipe = NULL;
+ }
}
}
@@ -3537,9 +3544,6 @@ ConnStateData::start()
BodyProducer::start();
HttpControlMsgSink::start();
- // ensure a buffer is present for this connection
- in.maybeMakeSpaceAvailable();
-
if (port->disable_pmtu_discovery != DISABLE_PMTU_OFF &&
(transparent() || port->disable_pmtu_discovery ==
DISABLE_PMTU_ALWAYS)) {
#if defined(IP_MTU_DISCOVER) && defined(IP_PMTUDISC_DONT)
---
Regards.
Prashanth
On 9 February 2016 at 13:54, Alex Rousskov
<rousskov@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> [this should be on squid-dev instead]
>
> On 02/09/2016 01:20 PM, Prashanth Prabhu wrote:
>
>> Here's the behavior I have seen: When the connection is set up, the
>> buffer gets a size of 16KB (default). Squid reads from the socket,
>> parses the data, and then sends it towards c-icap as appropriate. Now,
>> as part of parsing the data, the buffer is NUL-terminated via a call
>> to c_str(). This NUL-termination, however, is not accounted for by an
>> increase in the "offset" (off) in the underlying MemBlob, therefore,
>> the offset and size go out of sync.
>
> Just to avoid a misunderstanding:
>
> * MemBlob does not have an "offset".
>
> * SBuf::off_ should not change when we are adding characters to SBuf
> because it is the start of the buffer, not the end of it.
>
> * A call to c_str() should not increase SBuf::len_ either because it
> does not add a new character to the SBuf object. That call just
> terminates the underlying buffer.
>
> Based on your comments below, I think I know what you mean by "go out of
> sync", but everything is as "in sync" as it can be when one adds
> termination characters that are not really there from SBuf::length()
> point of view. The bug is elsewhere.
>
>
>> MemBlob::canAppend() failing because
>> MemBlob::isAppendOffset() fails -- the 'off' and 'size' are not the
>> same due to the above c_str() call.
>
> Single-owner optimizations aside (a known TODO), the above is the
> desired behavior according to the documented c_str() guarantees:
>
>> * The returned value points to an internal location whose contents
>> * are guaranteed to remain unchanged only until the next call
>> * to a non-constant member function of the SBuf object.
>
> In other words, we cannot allow some _other_ SBuf object to overwrite
> our null-termination character in the MemBlob we share with that other SBuf.
>
> The high price for that strong guarantee is one of the reasons we should
> avoid c_str() calls in Squid code.
>
>
>> When canAppend() fails, a new
>> buffer is re-allocated. When this reallocation occurs, however, the
>> new size of the buffer is dependent on the size being reserved.
>
> If we are still talking about the I/O buffer (and not just some random
> SBuf string somewhere), then the I/O buffer _capacity_ should not shrink
> below a certain minimum, regardless of how much content the buffer has
> already stored. There should be some Squid code that ensures the minimum
> capacity of the I/O buffer used to read requests. If it is missing, it
> is a Squid bug.
>
>
>> As a temporary measure, I have an experimental change that checks
>> whether the body size is known and if known always reserves a large
>> enough size (currently 16K).
>
> It is difficult to discuss this without seeing your changes, but the
> reservation should probably be unconditional -- the I/O buffer capacity
> should always be at least 16KB (or whatever size we start with).
>
>
> HTH,
>
> Alex.
>
_______________________________________________
squid-users mailing list
squid-users@xxxxxxxxxxxxxxxxxxxxx
http://lists.squid-cache.org/listinfo/squid-users
