[squid-dev] [PATCH] Make Squid death due to overloaded helpers optional

[Amos Jeffries]

On 9/08/2016 3:14 a.m., Eduard Bagdasaryan wrote:
> This patch allows to configure Squid so that it reject overloaded helper
> requests instead of [default] crashing.
> 
> Added on-persistent-overload=action option to helpers. Helper overload
> is defined as running with an overflowing queue. Persistent helper
> overload is [still] defined as being overloaded for more than 3 minutes.
> 
> The default behavior is unchanged(*) -- Squid worker dies with a fatal
> error at the attempt to submit a new request to a persistenly overloaded
> helper. This default behavior can also be configured explicitly using
> on-persistent-overload=die.
> 
> With on-persistent-overload=err, when dealing with a persistently
> overloaded helper, Squid immediately drops the helper request and sends
> an empty response to the caller which should handle that empty response
> as an error. Squid informs the admin when it starts and when it stops
> dropping helper requests due to persistent overload.
> 
> The code had conflicting notions of an "overloaded helper". The external
> ACL helper, the URL rewriter, and the store ID code used queueFull() to
> test whether the new request would overflow the queue (and, hence,
> overload the helper), but queueFull() itself did not check whether the
> queue was full! It checked whether the queue was already overflowing.
> This confusion resulted in that code scheduling one extra helper request
> before enabling bypass. The code and its documentation are now more
> consistent (and better match the "overload" terminology used by the new
> configuration option, which also feels better than calling the helper
> "full").
> 
> (*) Resolving the above confusion resulted in minor (one request)
> differences in the number of helper requests queued by Squid for
> external ACL, URL rewriting, and store ID helpers, with the adjusted
> behavior [better] matching the documentation.
> 
> Regards,
> Eduard.
> 

Thank you

in src/cf.data.pre:

* s/temporary exceed/temporarily exceed/

* For things like:
  "on-persistent-overload option, described below, applies"

 - please remove the above/below wording. The online config
documentation uses separate pages for options, and the config generator
can re-order directives sometimes. There is no dependable above/below to
refer to.
 - above/below occurs several times in the new texts.


* the lines documenting 'die' and 'err' action look a bit squashed up.
Please:
 - move "Two actions are supported:" to the start of a new line
 - empty lines for clarity on listed entries.
 - use 2 space indent on the action.
 - tab indent all the description lines to make sure that all lines
align on their left side.
 - remove the colons, that avoids anyone being confused about whether
they are part of the options name.

It should look like this:

"
	.....

	Two on-persistent-overload actions are supported:

	  die	...

	  err	...
		...
"



in src/helper.cc

* "whether at least one more request can be successfully submitted"
 - looks wrong documentation for the overloaded() method.
 - the true output happens when the queue *is* past full, not when one
more requests can be added.

* why is helper::SubmissionFailure() a member of helper?
 - it does not use any of *this class members.
 - you could make it a templated local static function:
 template<class Type> static void
 SubmissionFailure(const Type hlp, ...

* helper::SubmissionFailure is also changing what was previously
Helper::Unknown result codes to Helper::Error.
 - Helper::Error is one of the helper output codes, it means success.
Obviousy when hlp == NULL, there is no success going on.
 - please revert to sending helper::Unknown code.



Aso, can we please add a "wait" action as well. Which means just keep
queueing things and ignore the overload.


Cheers
Amos