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