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

[Alex Rousskov]

On 08/09/2016 05:38 AM, Eduard Bagdasaryan wrote:
> On 08/08/2016 02:17 PM, Amos Jeffries wrote:
>> * 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.

> Reverted. Note that for helper::Unknown some callers print a bit
> misleading error message, when requests are dropped, e.g.:
> "ERROR: storeID helper returned invalid result code. Wrong helper?"

and the documentation now lies:

> +		Two [on-persistent-overload] actions are supported:
> +
> +		  die	Squid worker quits. This is the default behavior.
> +
> +		  err	Squid treats the helper request as if it was
> +				immediately submitted, and the helper immediately 
> +				responded with an error. This action has no effect on
> +				the already queued and in-progress helper requests.

Perhaps SubmissionFailure() should use Helper::Unknown when its hlp
parameter is nil and Helper::Error in other cases?


FYI: The original intent and the documentation say that we should return
Helper::Error. Yes, Helper::Error means a "successfully parsed" helper
response [with helper-specific semantics], and IIRC, that is what we
wanted to relay to the helper-using code because that code has a better
chance of doing the right thing when receiving Helper::Error (compared
to handling Helper::Unknown).

We also considered adding a new result code for overload but rejected
that idea for several reasons, including the expectation that the
helper-using code already has logic to handle Helper::Error and that
logic will work for on-persistent-overload=err cases well.

Please note that I am not advocating a _change_ in the current Squid
default behavior. I am only questioning whether it is a good idea to
return Helper::Unknown instead of Helper::Error with
on-persistent-overload=err. Amos' arguments do not apply to that
specific case AFAICT because there is no "change" in that specific case
(it is a new behavior).


Thank you,

Alex.