Re: [PATCH] Fix session helper "crashing too rapidly"

 On Mon, 19 Sep 2011 23:45:58 +0100, Andrew Beverley wrote:
> On Mon, 2011-09-19 at 10:49 +1200, Amos Jeffries wrote:
>> The session helper in Squid-3 is concurrent.
>
> Ah, okay.
>
>> The user_key is the opaque
>> channel-ID. (Probably should be renamed to match the protocol
>> documentation).
>>
>> http://wiki.squid-cache.org/Features/AddonHelpers#Access_Control_.28ACL.29
>
> I've renamed to channel_id
>
>> The correct way to fix this is to detect the segfault case add a
>> stderr
>> ERROR: message that the helper is concurrent and requires a config
>> update.
>
> I've added a fatal error message to STDERR, but I can't get it to
> output
> anywhere (see below).
>
>> stderr should appear in cache.log whenever sent.
>
> Hmmm, it doesn't seem to be.
>
>> Most of the lines so
>> far appear to be debug messages
>
> Does that include the failure to open a database? This is written to
> STDERR as per any other message, but it does not make it anywhere.
>
>> , which depend on the -d option to
>> display.
>
> Do you mean the -d option to the Squid binary? If so, this doesn't
> seem
> to make any difference; it just prints all the log messages to the
> display as well as the log file.

 -d parameter of the helper binary. stderr is piped to cache.log at
 level-0. Thus the need for a separate switch to enable lots of debug
 from the helper.

>
> I've tried increasing the log level to 9, but to no avail.
>
>> Also....the .8 manual needs to mention the concurrency rather than
>> just
>> implying it in the example config.
>
> Done. Also, the following page should be updated:
>
> http://wiki.squid-cache.org/ConfigExamples/Portal/Splash
>
> I'm happy to do it myself, if you can give me wiki edit rights?

 Sure. Just need to know what username you login with.

>
>> The helper version should get a bump
>> to 1.1 as well.
>
> Done in the manual. Is it specified anywhere else as well?
>
> I've also added:
>
> - A new option: "-h". This causes a "hard" timeout, regardless of
> user
> activity. This is because, for many uses of a splash page (adverts
> etc)
> one would want the message to displayed every n hours, regardless of
> user activity (certainly that is my requirement).

 -h is reserved for display of "help" / usage() texts.

 -T would probably be better for an absolute timeout.

>
> - A db->sync command. I have found that with more than one child
> started, that they do not necessarily share data because each child's
> data has not been flushed to disk. This fixes that. However, I am not
> sure whether it has other implications, such as much greater disk
> overhead. Do you think it should be a configurable option?

 IMO that is just something that should happen. So not configurable
 unless we have to disable it for someone.

>
> Is there anything else that needs updating? RELEASENOTES.html?

 Not at this point. I'll get the Changelog entry during release
 processing.

>
> Please find attached updated patch for comment.

 Okay. The audit (ominous drums):

 ext_session_acl.8:
  * " (unless -h is specified).". Please make a separate sentence.
 Something like "Also supports fixed length timeouts for regular splash
 page displays."
   (if you can think of any other useful scenario than regular splash
 pages that can be mentioned too.)

  * rather than calling the new mode "Hard timeout". It's a "Periodic"
 or "Fixed length" timeout ... something a bit more descriptive like
 that.
  ** Needs to explain how it interacts with -a mode. ie I would expect
 LOGOUT or timeout to terminate the session. Explicit LOGIN required for
 a new one to start.

 ext_session_acl.cc: (modulo the -h ==> -T change requested)
  * in getopt() format syntax ':' indicates a value is received for the
 option. What you coded is "t:b:a?h?".
  * I think you should make -t and -T both optional arguments which are
 interchangeable. ("t:T:b:a?")
   case 'T':
     hard-timeout=1;
   case 't':
     ... process timeout value
     break;

 Other than that okay.

 Amos
Received on Tue Sep 20 2011 - 00:09:44 MDT