cancelling requests due to max_request_time

[Frank Cusack]

On January 15, 2007 9:20:07 PM +0100 Alan DeKok <aland at deployingradius.com> 
wrote:

> Frank Cusack wrote:
>> request_list.c around line 574, calling pthread_cancel() on the thread
>> handling a timed out request is a bad idea.  It can leave resources
>> hanging, like a locked mutex for ownership of an ldap file descriptor.
>
>   Yes.  The proper thing to do is....
>
>> In order for pthread_cancel() to be a good idea, most modules would
>> need to take care to disable cancellation while they hold resources
>> open, and/or set cancellation handlers to clean them up.  That's a
>> lot of work and there's a lot of room for bugs there.
>>
>> But I can't think of a better approach.  You don't want to let threads
>> just run to completion and waste resources.
>
>   That's what the STOP_NOW flag is for in struct REQUEST.  It should
> really be a 32-bit int of it's own, to prevent threading issues.  The
> code in src/main/modcall.c (or src/main/module.c) stops processing the
> request through the module list once that flag is set.

That's good, but doesn't help, pthread_cancel() is still called
for unresponsive threads.  I just noticed this in radiusd.conf:

  #  delete_blocked_requests: If the request takes MORE THAN 
'max_request_time'
  #  to be handled, then maybe the server should delete it.
  #
  #  If you're running in threaded, or thread pool mode, this setting
  #  should probably be 'no'.  Setting it to 'yes' when using a threaded
  #  server MAY cause the server to crash!
  #
  delete_blocked_requests = no

Is that right?  There are bugs there (besides pthread_cancel())?  I see
that if delete_block_requests is no, pthread_cancel() doesn't get
called and instead STOP_NOW is set, so that looks good since usu. a
request would be blocked on I/O, not compute, so waiting for a module
to complete some I/O shouldn't be an issue.

>   It doesn't bring a dead thread back to life, but it stops the request
> once it comes back.
>
>   Maybe an alternative is to do pthread_kill(),

No, because signals are handled process-wide.  Each thread would
have to have its own signal assigned to it, and then each
MODULE would have to implement the handler itself, because only
the module itself knows what resources it uses.  pthread_cancel()
would be better.

I would suggest simply removing the delete_blocked_requests option
(always use "no").  The STOP_NOW flag looks good enuf.

-frank