On January 15, 2007 9:20:07 PM +0100 Alan DeKok <aland@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