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

[Amos Jeffries]

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.

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.

Amos