cancelling requests due to max_request_time
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. 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. -frank
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. 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(), which a catchable signal. That may interrupt blocked system calls, but there's really no guarantee that the offending library won't simple retry. Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
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
On January 15, 2007 1:04:05 PM -0800 Frank Cusack <fcusack@fcusack.com> wrote: ...
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. ... I would suggest simply removing the delete_blocked_requests option (always use "no"). The STOP_NOW flag looks good enuf.
Thought more about this. Maybe a global cancel handler which calls into a new .cancel method of each module. If the module doesn't have this function it won't be cancelable. Hey I like that a lot. Just need to ensure delete_blocked_requests isn't actually buggy. -frank
Frank Cusack wrote:
Is that right? There are bugs there (besides pthread_cancel())?
Perhaps.
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.
Except for issues like DNS, or SQL servers being down, or LDAP being non-responsive for minutes. But yes, it will mostly work.
I would suggest simply removing the delete_blocked_requests option (always use "no"). The STOP_NOW flag looks good enuf.
OK.
Thought more about this. Maybe a global cancel handler which calls into a new .cancel method of each module. If the module doesn't have this function it won't be cancelable. Hey I like that a lot.
That's easy enough to do. But if a module is cancellable, odds are it won't be blocking on anything...
Just need to ensure delete_blocked_requests isn't actually buggy.
Now that I think about it, the request has to be deleted from the "live" list, and placed onto a "dead" list. That's so its memory isn't leaked. I spent some time trying to come up with a good & easy solution, but it never went anywhere. The "packet list" stuff in CVS head is a start, but not completely fleshed out. Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
On January 16, 2007 1:29:31 PM +0100 Alan DeKok <aland@deployingradius.com> wrote:
Frank Cusack wrote:
Is that right? There are bugs there (besides pthread_cancel())?
Perhaps.
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.
Except for issues like DNS, or SQL servers being down, or LDAP being non-responsive for minutes. But yes, it will mostly work.
Those are the exact I/O issues that would typically be the problem. By "not an issue" I mean not an issue for FR to just wait for the module to timeout its request, return, and then the STOP_NOW flag to take effect, as opposed to cancelling the operation now; because there's not going to be load on FR while it's just blocked on I/O. ie, NOT having delete_blocked_requests shouldn't cause a cpu load problem for FR.
I would suggest simply removing the delete_blocked_requests option (always use "no"). The STOP_NOW flag looks good enuf.
OK.
Thought more about this. Maybe a global cancel handler which calls into a new .cancel method of each module. If the module doesn't have this function it won't be cancelable. Hey I like that a lot.
That's easy enough to do. But if a module is cancellable, odds are it won't be blocking on anything...
If a module is cancellable, it has a cancel handler that would close all open fd's and release mutexes. That's the point of the handler, to cleanly release those resources so it can be canceled.
Just need to ensure delete_blocked_requests isn't actually buggy.
Now that I think about it, the request has to be deleted from the "live" list, and placed onto a "dead" list. That's so its memory isn't leaked.
I spent some time trying to come up with a good & easy solution, but it never went anywhere. The "packet list" stuff in CVS head is a start, but not completely fleshed out.
Might even be able to do a lock-free implementation. -frank
participants (2)
-
Alan DeKok -
Frank Cusack