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

[Alex Rousskov]

On 08/09/2016 11:39 AM, Amos Jeffries wrote:
> On 10/08/2016 4:41 a.m., Alex Rousskov wrote:
>> 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).
>>
> 
> At the moment Helper::Unknown means a problem in Squid side of things
> (fail). BH means a problem internally to the helper (fail). And both
> OK/ERR mean helper decision was made (success).
>  The naming may not great, and the way they map down to boolean
> allow/deny type values is very painful, but thats what we have to work
> with currently.

Yep, that matches both my understanding and motivation to return ERR in
the explicitly configured on-persistent-overload=err case.


> The decision of whether Unknown maps to ERR is something each different
> helper interface needs to decide for itself based on what its purpose is.

The decision what fake response object to give the helper code [when
helper itself is overloaded] should be up to the admin. The admin wants
"as if the helper returned ERR". We can (and perhaps should) add more
on-persistent-overload values later, of course.

Alex.