Re: [PATCH] Prevent external_acl.cc "inBackground" assertion on queue overloads

On 01/11/2013 09:42 PM, Amos Jeffries wrote:
> On 12/01/2013 1:39 p.m., Alex Rousskov wrote:
>> And should we use n_running or n_active for these checks?

> I think we should get away from the 2*N logic entirely and do something
> a bit safer.
> As it stands n_active AFAIK is the intended measure. n_running includes
> helpers being shutdown, so when they disappear the queue magically
> overflows. However if any one of the n_active helpers has a problem they
> get removed from n_active and the queue again magically overflows.

Sounds like neither "current n_active" nor "current n_running" is a good
measure for overflows because both may go down rather abruptly and
without admin control.

> Perhapse something like 2*n_max would be better, at least that is a
> fixed lengh of queue independent of what is happening with the helpers.

I am not a helper expert, but stable and directly-configurable n_max
does make more sense than dynamic and hidden n_running/n_active IMO.

In addition to that, I know of at least one use case where they have to
patch Squid sources because they believe their helpers need a different
limit than 2*n_something (and perhaps to fix some inconsistencies).

>> TODO: There are approximately ten places where we check queue sizes
>> against n_running or n_active. Many of those checks are slightly
>> different. This inconsistency should be removed, probably by adding a
>> few standard methods for a few types of checks that are actually needed.
>> Patches welcome!

> +1 on that idea.

I suggest to:

  1. Make the queue limit configurable, with the default set to 2*n_max.
  2. Move common queue limit checks into a few methods.
  3. Make all code to use those new methods.

With such a combination, Factory may even be able to sponsor the
development.

Any better ideas?

Thank you,

Alex.
Received on Sat Jan 12 2013 - 17:11:14 MST