Bad interaction between max_stale and negative caching (2.HEAD)
I've got a user who's running a pair of peered accelerators, using both stale-while-revalidate and max_stale. Occasionally, they see extremely old content being served; e.g., if CC: max-age is 60s, they might see something go by which is 1000-3000 seconds old (but still within the max_stale window). The pattern that appears to trigger this is when a resource with an in- cache 200 response starts returning 404s; when this happens, Squid will start returning TCP_NEGATIVE_HIT/200's. E.g. (traffic driven by squidclient), 1713703.815 0 127.0.0.1 TCP_STALE_HIT/200 5234 GET http:// server1//5012904 - NONE/- application/json 1221713703.979164 0.0.0.0 TCP_ASYNC_MISS/404 193 GET http:// server1/5012904 - FIRST_UP_PARENT/back-end-server1 text/plain 1221713711.431 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server 1/5012904 - NONE/- application/json 1221713720.978 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server 1/5012904 - NONE/- application/json 1221713723.483 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server 1/5012904 - NONE/- application/json As you can see, stale-while-revalidate kicks in, and the async refresh brings back a 404, but that doesn't get stored properly. Looking at the code, I *think* the culprit is storeNegativeCache(), which will, assuming that max_stale is set (either in squid.conf or response headers), block the new response from updating the cache -- no matter what its status code is. It makes sense to do this for 5xx status codes, because they're often transient, and reflect server-side problems. It doesn't make as much sense to do this for 4xx status codes, which reflect client-side issues. In those cases, you always want to update the cache with the most recent response (and potentially negative cache it, if the server is silly enough to not put a freshness lifetime on it). The interesting thing, BTW, is that this only happens when collapsed forwarding is on, because this in httpReplyProcessHeader: if (neighbors_do_private_keys ! Config.onoff.collapsed_forwarding) httpMaybeRemovePublic(entry, reply); masks this behaviour. Thoughts? I'm not 100% on this diagnosis, as the use of peering and stale-while-revalidate make things considerably more complex, but I've had pretty good luck reproducing it... I'm happy to attempt a fix, but wanted input on what approach people preferred. Left to my own devices, I'd add another condition to this in storeNegativeCache(): if (oe !EBIT_TEST(oe-flags, KEY_PRIVATE) !EBIT_TEST(oe-flags, ENTRY_REVALIDATE)) to limit it to 5xx responses. -- Mark Nottingham [EMAIL PROTECTED]
Merge of SBuf (formerly known as String class) begun
Hi all, I've branched off trunk to start merging the work we've extensively discussed in the past month into Squid. The feature branch is at https://code.launchpad.net/~kinkie/squid/stringng The previous codebase at lp:~kinkie/squid/string-ng-sandbox remains mostly as a changelog placeholder; its outcome has been placed in include/SBuf.h (to be split into .cc and .cci); I'll update the blueprint at http://wiki.squid-cache.org/Features/BetterStringBuffer Real Soon Now (tm). -- /kinkie
auto-tools files update
We've just had a submission adding DragonflyBSD support to the code. The contributor notes that the config.sub and config.guess files distributed with Squid are outdated and need to be upgraded for the new OS support to work. Anyone know how/what is needed to be done to the bootstrap system to get them into the source? Is it worth doing? Ref: http://www.squid-cache.org/bugs/show_bug.cgi?id=2465 Amos -- Please use Squid 2.7.STABLE4 or 3.0.STABLE9
Re: Bad interaction between max_stale and negative caching (2.HEAD)
Mark Nottingham wrote: I've got a user who's running a pair of peered accelerators, using both stale-while-revalidate and max_stale. Occasionally, they see extremely old content being served; e.g., if CC: max-age is 60s, they might see something go by which is 1000-3000 seconds old (but still within the max_stale window). The pattern that appears to trigger this is when a resource with an in-cache 200 response starts returning 404s; when this happens, Squid will start returning TCP_NEGATIVE_HIT/200's. E.g. (traffic driven by squidclient), 1713703.815 0 127.0.0.1 TCP_STALE_HIT/200 5234 GET http://server1//5012904 - NONE/- application/json 1221713703.979164 0.0.0.0 TCP_ASYNC_MISS/404 193 GET http://server1/5012904 - FIRST_UP_PARENT/back-end-server1 text/plain 1221713711.431 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server1/5012904 - NONE/- application/json 1221713720.978 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server1/5012904 - NONE/- application/json 1221713723.483 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server1/5012904 - NONE/- application/json As you can see, stale-while-revalidate kicks in, and the async refresh brings back a 404, but that doesn't get stored properly. Looking at the code, I *think* the culprit is storeNegativeCache(), which will, assuming that max_stale is set (either in squid.conf or response headers), block the new response from updating the cache -- no matter what its status code is. It makes sense to do this for 5xx status codes, because they're often transient, and reflect server-side problems. It doesn't make as much sense to do this for 4xx status codes, which reflect client-side issues. In those cases, you always want to update the cache with the most recent response (and potentially negative cache it, if the server is silly enough to not put a freshness lifetime on it). The interesting thing, BTW, is that this only happens when collapsed forwarding is on, because this in httpReplyProcessHeader: if (neighbors_do_private_keys !Config.onoff.collapsed_forwarding) httpMaybeRemovePublic(entry, reply); masks this behaviour. Thoughts? I'm not 100% on this diagnosis, as the use of peering and stale-while-revalidate make things considerably more complex, but I've had pretty good luck reproducing it... I'm happy to attempt a fix, but wanted input on what approach people preferred. Left to my own devices, I'd add another condition to this in storeNegativeCache(): if (oe !EBIT_TEST(oe-flags, KEY_PRIVATE) !EBIT_TEST(oe-flags, ENTRY_REVALIDATE)) to limit it to 5xx responses. I'd agree with you based on that analysis. Can you add a bugzilla entry with a patch that does it? Amos -- Please use Squid 2.7.STABLE4 or 3.0.STABLE9
Re: Bad interaction between max_stale and negative caching (2.HEAD)
Will do tomorrow. On 18/09/2008, at 10:39 PM, Amos Jeffries wrote: Mark Nottingham wrote: I've got a user who's running a pair of peered accelerators, using both stale-while-revalidate and max_stale. Occasionally, they see extremely old content being served; e.g., if CC: max-age is 60s, they might see something go by which is 1000-3000 seconds old (but still within the max_stale window). The pattern that appears to trigger this is when a resource with an in-cache 200 response starts returning 404s; when this happens, Squid will start returning TCP_NEGATIVE_HIT/200's. E.g. (traffic driven by squidclient), 1713703.815 0 127.0.0.1 TCP_STALE_HIT/200 5234 GET http:// server1//5012904 - NONE/- application/json 1221713703.979164 0.0.0.0 TCP_ASYNC_MISS/404 193 GET http://server 1/5012904 - FIRST_UP_PARENT/back-end-server1 text/plain 1221713711.431 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server 1/5012904 - NONE/- application/json 1221713720.978 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server 1/5012904 - NONE/- application/json 1221713723.483 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server 1/5012904 - NONE/- application/json As you can see, stale-while-revalidate kicks in, and the async refresh brings back a 404, but that doesn't get stored properly. Looking at the code, I *think* the culprit is storeNegativeCache(), which will, assuming that max_stale is set (either in squid.conf or response headers), block the new response from updating the cache -- no matter what its status code is. It makes sense to do this for 5xx status codes, because they're often transient, and reflect server-side problems. It doesn't make as much sense to do this for 4xx status codes, which reflect client- side issues. In those cases, you always want to update the cache with the most recent response (and potentially negative cache it, if the server is silly enough to not put a freshness lifetime on it). The interesting thing, BTW, is that this only happens when collapsed forwarding is on, because this in httpReplyProcessHeader: if (neighbors_do_private_keys ! Config.onoff.collapsed_forwarding) httpMaybeRemovePublic(entry, reply); masks this behaviour. Thoughts? I'm not 100% on this diagnosis, as the use of peering and stale-while-revalidate make things considerably more complex, but I've had pretty good luck reproducing it... I'm happy to attempt a fix, but wanted input on what approach people preferred. Left to my own devices, I'd add another condition to this in storeNegativeCache(): if (oe !EBIT_TEST(oe-flags, KEY_PRIVATE) !EBIT_TEST(oe- flags, ENTRY_REVALIDATE)) to limit it to 5xx responses. I'd agree with you based on that analysis. Can you add a bugzilla entry with a patch that does it? Amos -- Please use Squid 2.7.STABLE4 or 3.0.STABLE9 -- Mark Nottingham [EMAIL PROTECTED]
Re: auto-tools files update
On Fri, 2008-09-19 at 00:33 +1200, Amos Jeffries wrote: We've just had a submission adding DragonflyBSD support to the code. The contributor notes that the config.sub and config.guess files distributed with Squid are outdated and need to be upgraded for the new OS support to work. Anyone know how/what is needed to be done to the bootstrap system to get them into the source? Is it worth doing? Ref: http://www.squid-cache.org/bugs/show_bug.cgi?id=2465 We bootstrap on FreeBSD, right? I would just wait for FreeBSD autoconf package to include the needed changes. If the current package is not up-to-date with what FreeBSD already provides, try contacting [EMAIL PROTECTED] Cheers, Alex.
SBuf review
On Thu, 2008-09-18 at 14:10 +0200, Kinkie wrote: The feature branch is at https://code.launchpad.net/~kinkie/squid/stringng Here is another round of comments, based on the current SBuf.h in the above repository. I have marked my comments with these prefixes: *B1*: a blocking issue. I do not recommend proceeding much further until this issue is resolved because the code may not be accepted or would have to be changed significantly to address the issue. *B2*: I am not sure yet, but this may become a blocker for me. If you are not certain, please consider save us some time by implementing the requested changes. If you are certain that I am wrong, a discussion would be nice and may save us from trouble later. *B3*: Everything else. *B1* I still think that string-manipulation interfaces should be moved into a String class. *B2* I still think that having NULL Strings is a bad idea. *B2* I am not sure placing your classes in include/ is better than placing them in src/. We have discussed similar questions before. I do not know whether there was consensus, but I hope we agreed that only Squid-independent portability/wrapper code should live outside of src/. I think your code will (or should) be Squid-dependent. It will make a lot of things easier. It is difficult to develop outside of src/ and it does not buy us anything in this case. *B2* class outOfBoundsException: public std::exception It is better to inherit all your customized exceptions from the existing TextException. Otherwise, the catching code will become unnecessary long and the exception class code will duplicate a lot of things. TextException itself should be polished and split or renamed to Exception, but that is a different topic. Most generic catching code should catch TextExceptions (which includes their children). *B1* Exception specifications: type method(...) throw(...); Surprisingly (well, at least it was to me!), exception specifications are a bad idea in virtually all cases except for empty specifications. Empty specifications should also be avoided unless you Know What You Are Doing and Absulutely Sure About That. Please remove all your empty throw() exception specifications. Please comment out (but do not remove) your non-empty exception specifications. They are useful as documentation: type method(...); // throw(...) or perhaps type method(...); // throws ... Thinking I am nuts? Here is one of the authoritative references on this fascinating subject: http://www.gotw.ca/publications/mill22.htm *B1* outOfBoundsException::what() returns a string that does not exist outside of that method. One more reason to move non-trivial code outside of the header where we do not have to review it (yet) :-). *B2* Move helper class declarations outside of SBuf. Class is not a namespace. Nested classes complicate the overall impression from the API and tempt you to do things you really should not. *B1* dump() methods should be constant. *B3* If a class does not have both dump() and print() methods, let's call the only printing method print(). This will make it easier to manipulate printable objects for debugging. *B2* If SBufStore is refcounted, why is it not using the RefCountable API we already have? If this is some kind of optimization, I would prefer to see it done after the code is reviewed, merged, and debugged. *B2* Please make SBufStore data members private. They are too sensitive to expose them like that. With the sensitive info open, we will not catch all the abuses (or spend too much time explaining why a public interface should not be used). *B3* Please rename SBufStore::init() to a more traditional allocate(). Even your own comment says that the block is not initialized! *B2* init() is missing asserts that the buffer has not been allocated twice. If that code is meant to be called immediately from constructors only, then at least make it private and add a comment about that. It is important to note that the object is in invalid state until init() has been called. *B1* SBufStore::init() and SBufStore in general should not change the reference counter. Reference counting is the prerogative of smart pointers, not the objects they point to (even if the object stores the counter). You need to add a smart pointer so that the code becomes clear and manageable. Again, I suggest using RefCountable API to start with. It will be easy to replace/optimize later, with no significant effect on user code. Again, this is why I keep suggesting that a polished API is agreed on first, before we start finding bugs in the unpolished API implementation. *B2* SBufStore constructors should be declared before init() and other methods. Same for SBuf. *B1* SBufStore(const SBuf S) is not a copy constructor but the comment says it is. *B2* SBufStore(const SBuf S) makes implicit conversions possible. Do we really need these? Doesn't creating store from a buffer that uses that store make you feel uncomfortable? *B1*
Re: [MERGE] Finish forward-porting HTCP enhancements from squid 2.HEAD.
On Thu, 2008-09-18 at 13:00 +1000, Benno Rice wrote: This is a forward-port of the HTCP changes I made to squid 2.HEAD. Changes include: - Ability to send HTCP CLR requests when objects are invalidated or purged from the cache. - Config logic to allow the following: - HTCP peers who ONLY receive CLR messages from us. - HTCP peers who NEVER receive CLR messages from us. - HTCP peers who NEVER receive CLR messages from us for PURGE requests. - HTCP peers who are forwarded CLR messages we receive. - Code to support all of the above. Please document htcpForwardClr() purpose. Are we forwarding something to all HTCP peers there? Please document htcpClear() purpose. Please document neighborsHtcpClear() purpose. If possible, please document htcpHandle() since you are modifying and renaming it. Here and above, I am just asking for a brief purpose comment before the function, for those who will need to debug it or see it in a stack trace. +htcpStuff stuff; Not your fault, but is there a more descriptive name for that object? :-) debug(15, 1) (neighborsHtcpClear: clear reason: %d\n, reason); There are a few statements logged at level 1, including statements inside loops. That may be perfectly fine, but please make sure you intended it that way. Thank you, Alex.
Re: SBuf review
On Thu, Sep 18, 2008 at 7:26 PM, Alex Rousskov [EMAIL PROTECTED] wrote: On Thu, 2008-09-18 at 14:10 +0200, Kinkie wrote: The feature branch is at https://code.launchpad.net/~kinkie/squid/stringng Here is another round of comments, based on the current SBuf.h in the above repository. I have marked my comments with these prefixes: *B1*: a blocking issue. I do not recommend proceeding much further until this issue is resolved because the code may not be accepted or would have to be changed significantly to address the issue. *B2*: I am not sure yet, but this may become a blocker for me. If you are not certain, please consider save us some time by implementing the requested changes. If you are certain that I am wrong, a discussion would be nice and may save us from trouble later. *B3*: Everything else. *B1* I still think that string-manipulation interfaces should be moved into a String class. I could agree to that if I could see an equally efficient way to implement this. I've called the class SBuf because I see it as being a bit of a String and a bit of a Buf. You may notice that there are NO functions which deal with encoding, for instance. I don't plan to add them to this class. *B2* I still think that having NULL Strings is a bad idea. Lazy instantiation? I can agree to a name change. But then it's also a pretty straightforward porting of a MemBuf construct with the same name. Reasoning goes, if it's in MemBuf there may be a valid reason for it. *B2* I am not sure placing your classes in include/ is better than placing them in src/. We have discussed similar questions before. I do not know whether there was consensus, but I hope we agreed that only Squid-independent portability/wrapper code should live outside of src/. I think your code will (or should) be Squid-dependent. It will make a lot of things easier. It is difficult to develop outside of src/ and it does not buy us anything in this case. I agree it should be Squid-dependent, as it will have to hook into MemPools, Debug, CacheMgr and various other places. I'm trying to follow the squid 3 coding guidelines as expressed in the wiki, but maybe I misunderstood them. I can move it to src/ no problem. *B2* class outOfBoundsException: public std::exception It is better to inherit all your customized exceptions from the existing TextException. Otherwise, the catching code will become unnecessary long and the exception class code will duplicate a lot of things. TextException itself should be polished and split or renamed to Exception, but that is a different topic. Most generic catching code should catch TextExceptions (which includes their children). I've followed recommended c++ coding guidelines, but I have no issues in implementing this. *B1* Exception specifications: type method(...) throw(...); Surprisingly (well, at least it was to me!), exception specifications are a bad idea in virtually all cases except for empty specifications. Empty specifications should also be avoided unless you Know What You Are Doing and Absulutely Sure About That. Please remove all your empty throw() exception specifications. Please comment out (but do not remove) your non-empty exception specifications. They are useful as documentation: type method(...); // throw(...) or perhaps type method(...); // throws ... Thinking I am nuts? Here is one of the authoritative references on this fascinating subject: http://www.gotw.ca/publications/mill22.htm Ok. Leaving them documented is the most important thing to me. I can also move them to doxygen comments so that they'll be more visible. *B1* outOfBoundsException::what() returns a string that does not exist outside of that method. One more reason to move non-trivial code outside of the header where we do not have to review it (yet) :-). *B2* Move helper class declarations outside of SBuf. Class is not a namespace. Nested classes complicate the overall impression from the API and tempt you to do things you really should not. If possible at all, I'd prefer not to. The reason why classes are placed like that is to enforce accessibility constraints without using friend classes as much as possible (see below) *B1* dump() methods should be constant. Trivial, will do. *B3* If a class does not have both dump() and print() methods, let's call the only printing method print(). This will make it easier to manipulate printable objects for debugging. The convention I followed is: dump() is for debugging, stats etc. print() is for operator . I can adhere to any other convention, as long as there's consensus - ideally to the point where it's sanctioned in the coding guidelines. *B2* If SBufStore is refcounted, why is it not using the RefCountable API we already have? If this is some kind of optimization, I would prefer to see it done after the code is reviewed, merged, and debugged. This code is very low-level. and it's also very self-contained. I aim
Re: [RFC] Policy Change on half-closed connection
Amos, your squid-users announcement says that it applies to 2.7 as well; will the same changes (switch default, remove later) be applied there? FWIW, I've never seen this happen on 2.x. Has anyone else? Note that turning half_closed_clients off has an effect on quick_abort (at least in 2.x); without it, if the client aborts before headers are received, quick_abort doesn't have time to kick in; with it, it will operate even if the client aborts that early. This behaviour is pretty desirable in an accelerator setup... Cheers, On 13/09/2008, at 6:52 PM, Amos Jeffries wrote: Basically bug 2431 http://www.squid-cache.org/bugs/show_bug.cgi?id=2431. Alex mentions seeing bad side effects and it being maybe unsuitable for a stable release. I posit that it is suitable to change the cause of a major bug in the stable release and a config default is safer than a core code change. As for further testing, we have a few users who have encountered the bug already and tested the config change. With good results. Henrik reports that many of his customers have done so as well. People who have explicitly set their configuration and are happy are not affected. For those who need to enable it, I think we have 3- HEAD and 2.7. Amos -- Please use Squid 2.7.STABLE4 or 3.0.STABLE9 -- Mark Nottingham [EMAIL PROTECTED]
Re: Bad interaction between max_stale and negative caching (2.HEAD)
Bug 2468; http://www.squid-cache.org/bugs/show_bug.cgi?id=2468 patch attached. I marked it as blocking 2255, as that's the bug for porting stale-if- error to 3.x. On 18/09/2008, at 10:39 PM, Amos Jeffries wrote: Mark Nottingham wrote: I've got a user who's running a pair of peered accelerators, using both stale-while-revalidate and max_stale. Occasionally, they see extremely old content being served; e.g., if CC: max-age is 60s, they might see something go by which is 1000-3000 seconds old (but still within the max_stale window). The pattern that appears to trigger this is when a resource with an in-cache 200 response starts returning 404s; when this happens, Squid will start returning TCP_NEGATIVE_HIT/200's. E.g. (traffic driven by squidclient), 1713703.815 0 127.0.0.1 TCP_STALE_HIT/200 5234 GET http:// server1//5012904 - NONE/- application/json 1221713703.979164 0.0.0.0 TCP_ASYNC_MISS/404 193 GET http://server 1/5012904 - FIRST_UP_PARENT/back-end-server1 text/plain 1221713711.431 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server 1/5012904 - NONE/- application/json 1221713720.978 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server 1/5012904 - NONE/- application/json 1221713723.483 0 127.0.0.1 TCP_NEGATIVE_HIT/200 5234 GET http://server 1/5012904 - NONE/- application/json As you can see, stale-while-revalidate kicks in, and the async refresh brings back a 404, but that doesn't get stored properly. Looking at the code, I *think* the culprit is storeNegativeCache(), which will, assuming that max_stale is set (either in squid.conf or response headers), block the new response from updating the cache -- no matter what its status code is. It makes sense to do this for 5xx status codes, because they're often transient, and reflect server-side problems. It doesn't make as much sense to do this for 4xx status codes, which reflect client- side issues. In those cases, you always want to update the cache with the most recent response (and potentially negative cache it, if the server is silly enough to not put a freshness lifetime on it). The interesting thing, BTW, is that this only happens when collapsed forwarding is on, because this in httpReplyProcessHeader: if (neighbors_do_private_keys ! Config.onoff.collapsed_forwarding) httpMaybeRemovePublic(entry, reply); masks this behaviour. Thoughts? I'm not 100% on this diagnosis, as the use of peering and stale-while-revalidate make things considerably more complex, but I've had pretty good luck reproducing it... I'm happy to attempt a fix, but wanted input on what approach people preferred. Left to my own devices, I'd add another condition to this in storeNegativeCache(): if (oe !EBIT_TEST(oe-flags, KEY_PRIVATE) !EBIT_TEST(oe- flags, ENTRY_REVALIDATE)) to limit it to 5xx responses. I'd agree with you based on that analysis. Can you add a bugzilla entry with a patch that does it? Amos -- Please use Squid 2.7.STABLE4 or 3.0.STABLE9 -- Mark Nottingham [EMAIL PROTECTED]
Re: SBuf review
On Thu, 2008-09-18 at 22:14 +0200, Kinkie wrote: *B1* I still think that string-manipulation interfaces should be moved into a String class. I could agree to that if I could see an equally efficient way to implement this. I've called the class SBuf because I see it as being a bit of a String and a bit of a Buf. This is not an efficiency issue, IMO. The all-in-one and separate-and-conquer designs, if done correctly, have similar overheads. *B2* I still think that having NULL Strings is a bad idea. Lazy instantiation? Lazy instantiation is an implementation detail. I am talking about the API. You can instantiate lazily with or without nil strings. I can agree to a name change. But then it's also a pretty straightforward porting of a MemBuf construct with the same name. Reasoning goes, if it's in MemBuf there may be a valid reason for it. This is not about a name or the old C code that we should not mimic. It is about introducing an invalid and unusual state that coders will forget about and will have to bother with. The code will be _better_ if isNull is removed. It will be worse if it stays. *B2* I am not sure placing your classes in include/ is better than placing them in src/. We have discussed similar questions before. I do not know whether there was consensus, but I hope we agreed that only Squid-independent portability/wrapper code should live outside of src/. I think your code will (or should) be Squid-dependent. It will make a lot of things easier. It is difficult to develop outside of src/ and it does not buy us anything in this case. I agree it should be Squid-dependent, as it will have to hook into MemPools, Debug, CacheMgr and various other places. I'm trying to follow the squid 3 coding guidelines as expressed in the wiki, but maybe I misunderstood them. I can move it to src/ no problem. Which wiki page? I do not see anything related at http://wiki.squid-cache.org/Squid3CodingGuidelines *B2* class outOfBoundsException: public std::exception It is better to inherit all your customized exceptions from the existing TextException. Otherwise, the catching code will become unnecessary long and the exception class code will duplicate a lot of things. TextException itself should be polished and split or renamed to Exception, but that is a different topic. Most generic catching code should catch TextExceptions (which includes their children). I've followed recommended c++ coding guidelines, but I have no issues in implementing this. Please do. We may make std::exception a parent of Exception, but that is a different (and minor) issue. *B1* Exception specifications: type method(...) throw(...); Surprisingly (well, at least it was to me!), exception specifications are a bad idea in virtually all cases except for empty specifications. Empty specifications should also be avoided unless you Know What You Are Doing and Absulutely Sure About That. Please remove all your empty throw() exception specifications. Please comment out (but do not remove) your non-empty exception specifications. They are useful as documentation: type method(...); // throw(...) or perhaps type method(...); // throws ... Thinking I am nuts? Here is one of the authoritative references on this fascinating subject: http://www.gotw.ca/publications/mill22.htm Ok. Leaving them documented is the most important thing to me. I can also move them to doxygen comments so that they'll be more visible. It is your call. Some of us find doxygen comments less readable :-). *B2* Move helper class declarations outside of SBuf. Class is not a namespace. Nested classes complicate the overall impression from the API and tempt you to do things you really should not. If possible at all, I'd prefer not to. The reason why classes are placed like that is to enforce accessibility constraints without using friend classes as much as possible (see below) I doubt you will need friend classes to implement a clean API in this case. However, I am certain that a lot of the current mess will be resolved on its own when you are forced to think about natural class boundaries. *B3* If a class does not have both dump() and print() methods, let's call the only printing method print(). This will make it easier to manipulate printable objects for debugging. The convention I followed is: dump() is for debugging, stats etc. print() is for operator . There is no such convention because for operator does not define method semantics. I am not going to argue about this B3 issue though. We have a bigger fish to catch... *B2* If SBufStore is refcounted, why is it not using the RefCountable API we already have? If this is some kind of optimization, I would prefer to see it done after the code is reviewed, merged, and debugged. This code is very low-level. and it's also very self-contained. I aim for maximum efficiency. We have gone
Semantics of ENTRY_NEGCACHED
While digging around to create bug 2468, I started wondering about what ENTRY_NEGCACHED really means. I interpret negative caching as using a heuristic -- namely, that something is an HTTP error -- to make something cacheable. This is why I don't complain about the neg caching code being surrounded in #if HTTP_VIOLATIONS. However, error responses can have an explicit freshness lifetime set in their headers, just like normal responses. In these cases, it seems to me that we shouldn't mark them as ENTRY_NEGCACHED, because we're *not* taking any liberty with their freshness. I.e., I'm wondering if it makes sense to move EBIT_SET(e-flags, ENTRY_NEGCACHED); to be part of if (expires == -1) expires = squid_curtime + cc.negative_ttl; in storeNegativelyCache. The most visible impact of this is that a 404 (etc.) response with a Cache-Control: max-age (or similar) would show up in logs as a TCP_HIT rather than TCP_NEGATIVE_HIT. From browsing the code, there would be a number of other smaller but I think sensible impacts (e.g., it can be put into a digest, and stats are cleaned up). Thoughts? -- Mark Nottingham [EMAIL PROTECTED]
Re: SBuf review
Kinkie wrote: On Thu, Sep 18, 2008 at 7:26 PM, Alex Rousskov [EMAIL PROTECTED] wrote: On Thu, 2008-09-18 at 14:10 +0200, Kinkie wrote: The feature branch is at https://code.launchpad.net/~kinkie/squid/stringng Here is another round of comments, based on the current SBuf.h in the above repository. I have marked my comments with these prefixes: *B1*: a blocking issue. I do not recommend proceeding much further until this issue is resolved because the code may not be accepted or would have to be changed significantly to address the issue. *B2*: I am not sure yet, but this may become a blocker for me. If you are not certain, please consider save us some time by implementing the requested changes. If you are certain that I am wrong, a discussion would be nice and may save us from trouble later. *B3*: Everything else. *B1* I still think that string-manipulation interfaces should be moved into a String class. I could agree to that if I could see an equally efficient way to implement this. I've called the class SBuf because I see it as being a bit of a String and a bit of a Buf. You may notice that there are NO functions which deal with encoding, for instance. I don't plan to add them to this class. I kind of fuzzily disagree, the point of this is to replace MemBuf + String with SBuf. Not implement both again independently duplicating stuff. *B2* I still think that having NULL Strings is a bad idea. Lazy instantiation? I can agree to a name change. But then it's also a pretty straightforward porting of a MemBuf construct with the same name. Reasoning goes, if it's in MemBuf there may be a valid reason for it. Agnostic. One of the open bugs is to clear up handling of NULL strings. They are needed in abstract for some use-cases such a URI handling, where parts of the URI may be missing. Whether they need to be NULL or 0-length is iffy. *B2* I am not sure placing your classes in include/ is better than placing them in src/. We have discussed similar questions before. I do not know whether there was consensus, but I hope we agreed that only Squid-independent portability/wrapper code should live outside of src/. I think your code will (or should) be Squid-dependent. It will make a lot of things easier. It is difficult to develop outside of src/ and it does not buy us anything in this case. I agree it should be Squid-dependent, as it will have to hook into MemPools, Debug, CacheMgr and various other places. I'm trying to follow the squid 3 coding guidelines as expressed in the wiki, but maybe I misunderstood them. I can move it to src/ no problem. I looked at this for IPAddress. It's rather tricky to extend things in include/ safely, so src/ is preferred for everything not an OS compatibility code. Hopefully in a new sub-folder (src/buffers/ ?) consistent with the SourceLayout plans. *B2* class outOfBoundsException: public std::exception It is better to inherit all your customized exceptions from the existing TextException. Otherwise, the catching code will become unnecessary long and the exception class code will duplicate a lot of things. TextException itself should be polished and split or renamed to Exception, but that is a different topic. Most generic catching code should catch TextExceptions (which includes their children). I've followed recommended c++ coding guidelines, but I have no issues in implementing this. *B1* Exception specifications: type method(...) throw(...); Surprisingly (well, at least it was to me!), exception specifications are a bad idea in virtually all cases except for empty specifications. Empty specifications should also be avoided unless you Know What You Are Doing and Absulutely Sure About That. Please remove all your empty throw() exception specifications. Please comment out (but do not remove) your non-empty exception specifications. They are useful as documentation: type method(...); // throw(...) or perhaps type method(...); // throws ... Thinking I am nuts? Here is one of the authoritative references on this fascinating subject: http://www.gotw.ca/publications/mill22.htm Ok. Leaving them documented is the most important thing to me. I can also move them to doxygen comments so that they'll be more visible. *B1* outOfBoundsException::what() returns a string that does not exist outside of that method. One more reason to move non-trivial code outside of the header where we do not have to review it (yet) :-). *B2* Move helper class declarations outside of SBuf. Class is not a namespace. Nested classes complicate the overall impression from the API and tempt you to do things you really should not. If possible at all, I'd prefer not to. The reason why classes are placed like that is to enforce accessibility constraints without using friend classes as much as possible (see below) *B1* dump() methods should be constant. Trivial, will do. *B3* If a class does not have both dump() and print() methods, let's call the
Re: Semantics of ENTRY_NEGCACHED
Mark Nottingham wrote: While digging around to create bug 2468, I started wondering about what ENTRY_NEGCACHED really means. I interpret negative caching as using a heuristic -- namely, that something is an HTTP error -- to make something cacheable. This is why I don't complain about the neg caching code being surrounded in #if HTTP_VIOLATIONS. However, error responses can have an explicit freshness lifetime set in their headers, just like normal responses. In these cases, it seems to me that we shouldn't mark them as ENTRY_NEGCACHED, because we're *not* taking any liberty with their freshness. I.e., I'm wondering if it makes sense to move EBIT_SET(e-flags, ENTRY_NEGCACHED); to be part of if (expires == -1) expires = squid_curtime + cc.negative_ttl; in storeNegativelyCache. The most visible impact of this is that a 404 (etc.) response with a Cache-Control: max-age (or similar) would show up in logs as a TCP_HIT rather than TCP_NEGATIVE_HIT. From browsing the code, there would be a number of other smaller but I think sensible impacts (e.g., it can be put into a digest, and stats are cleaned up). Thoughts? You have a good point. I don't think the POS/NEG should be combined though. NEGCACHED has clear info for debugging websites which return error pages. A better way would be to add a third state STALE or somesuch, for the actual violations. Just my 2c. Amos -- Please use Squid 2.7.STABLE4 or 3.0.STABLE9
Re: [RFC] Policy Change on half-closed connection
Mark Nottingham wrote: Amos, your squid-users announcement says that it applies to 2.7 as well; will the same changes (switch default, remove later) be applied there? I'm not sure, Henrik will make that decision. Though there is no bug reason for it to happen in 2.7. FWIW, I've never seen this happen on 2.x. Has anyone else? Not that Henrik is aware of. Thus the RFC. We just need it to be tested on all installs so we don't change defaults behind people backs, and rip the code out of 3.2 then find its needed by someone later who was on 2.x today. Note that turning half_closed_clients off has an effect on quick_abort (at least in 2.x); without it, if the client aborts before headers are received, quick_abort doesn't have time to kick in; with it, it will operate even if the client aborts that early. This behaviour is pretty desirable in an accelerator setup... Noted. Thank you. That may just be the very reason for keeping it coded, but in off state by default. Cheers, On 13/09/2008, at 6:52 PM, Amos Jeffries wrote: Basically bug 2431 http://www.squid-cache.org/bugs/show_bug.cgi?id=2431. Alex mentions seeing bad side effects and it being maybe unsuitable for a stable release. I posit that it is suitable to change the cause of a major bug in the stable release and a config default is safer than a core code change. As for further testing, we have a few users who have encountered the bug already and tested the config change. With good results. Henrik reports that many of his customers have done so as well. People who have explicitly set their configuration and are happy are not affected. For those who need to enable it, I think we have 3-HEAD and 2.7. Amos -- Please use Squid 2.7.STABLE4 or 3.0.STABLE9
Re: SBuf review
On Fri, 2008-09-19 at 14:36 +1200, Amos Jeffries wrote: *B3* If a class does not have both dump() and print() methods, let's call the only printing method print(). This will make it easier to manipulate printable objects for debugging. The convention I followed is: dump() is for debugging, stats etc. print() is for operator . I can adhere to any other convention, as long as there's consensus - ideally to the point where it's sanctioned in the coding guidelines. The whole stats output dump() system IIRC was partially completed moving to sstream . So a specific sstream and ostream 'friends' might be better than print() and dump() anyways. Side note: Friends cannot be virtual and templated so, in general, print() is a good idea, usually accompanied by a [templated] operator to print anything printable. We probably need to agree on the definition/purpose of print(), dump(), and other similar methods, but that is a separate question. *B2* If SBufStore is refcounted, why is it not using the RefCountable API we already have? If this is some kind of optimization, I would prefer to see it done after the code is reviewed, merged, and debugged. This code is very low-level. and it's also very self-contained. I aim for maximum efficiency. Ditto on the RefCountable. This was the API I was asuming you would use anyway. It's already well debugged, and tested, and low-level inline. Just inherit from RefCount and use I think. If we find in testing that we need the efficiency, we can test the two. RefCount efficiency is about the same as that of the proposed code. I did not realize that when I made the original comment above. I forgot that RefCountable stores a counter and was willing to trade a little inefficiency for correct code. After looking at RefCountable, I can now confidently mark the above issue as *B1*! *B1* SBufStore is missing a copy constructor and an assignment operator. You do not have to actually implement them, but you have to declare them to make sure the defaults are not used. Make them private if you think they should never be used. With a fatal assert or loud DBG_CRITICAL debugs() complaint to catch bad behavior. I have seen folks declaring but not defining the forbidden methods. With that approach, violations lead to link-stage errors, which is much better than a runtime assert or debug. However, I do not know how portable such trick is. Does anybody know? On the other hand, I think we already use that in Squid so we can continue doing so until caught (not defining something does not add extra work). *B1* SBuf assignment does not handle assignment to self. Right. Is it safe to implement that as a noop? Worst case, do it with a fatal assertion and we will find out in testing. Please do not assert! Assignment to self is acceptable and unavoidable when you do not know whether pointer/reference1 points/refers to the same object as pointer/reference2: a = b; // this can be an assignment to self! The best code supports assignment to self without a special if-statement that checks for assignment to self. Such code is difficult to write though and our standards are not that high. A simple (this == them) check would do in most cases, I think. HTH, Alex.
Re: [RFC] Policy Change on half-closed connection
On Fri, 2008-09-19 at 09:16 +1000, Mark Nottingham wrote: Amos, your squid-users announcement says that it applies to 2.7 as well; will the same changes (switch default, remove later) be applied there? FWIW, I've never seen this happen on 2.x. Has anyone else? Note that turning half_closed_clients off has an effect on quick_abort (at least in 2.x); without it, if the client aborts before headers are received, quick_abort doesn't have time to kick in; with it, it will operate even if the client aborts that early. This behaviour is pretty desirable in an accelerator setup... Sounds like a big enough problem to leave Squid2 settings alone! This scenario should also be tested with Squid3. Thank you, Alex. On 13/09/2008, at 6:52 PM, Amos Jeffries wrote: Basically bug 2431 http://www.squid-cache.org/bugs/show_bug.cgi?id=2431. Alex mentions seeing bad side effects and it being maybe unsuitable for a stable release. I posit that it is suitable to change the cause of a major bug in the stable release and a config default is safer than a core code change. As for further testing, we have a few users who have encountered the bug already and tested the config change. With good results. Henrik reports that many of his customers have done so as well. People who have explicitly set their configuration and are happy are not affected. For those who need to enable it, I think we have 3- HEAD and 2.7. Amos -- Please use Squid 2.7.STABLE4 or 3.0.STABLE9 -- Mark Nottingham [EMAIL PROTECTED]
Re: SBuf review
2008/9/19 Amos Jeffries [EMAIL PROTECTED]: I kind of fuzzily disagree, the point of this is to replace MemBuf + String with SBuf. Not implement both again independently duplicating stuff. I'll say it again - ignore MemBuf. Ignore MemBuf for now. Leave it as a NUL-terminated dynamic buffer with some printf append like semantics. When you've implemented a non-NUL-terminated ref-counted memory region implementation and you layer some basic strings semantics on top of it, you can slowly convert or eliminate the bulk of the MemBuf users over. You're going to find plenty of places where the string handling is plain old horrible. Don't try to cater for those situations with things like NULL strings. I tried that, its ugly. Aim to implement something which'll cater to something narrow to begin with - like parsing HTTP headers - and look to -rewrite- larger parts of the code later on. Don't try to invent things which will somehow seamlessly fit into the existing code and provide the same semantics. Some of said semantics is plain shit. I still don't get why this is again becoming so freakishly complicated. Adrian
