On 03/21/2013 11:45 AM, Tommie Gannert wrote:
2013/3/21 Alexander Klauer <alexander.kla...@itwm.fraunhofer.de>:
An invocation of ares_cancel() walks through the request list, calling
the callbacks of all pending requests on a channel. Previously, if such
a callback added a new request to the channel, the request list might
not end up empty, causing an abort by assertion failure. The present
commit ensures that all such newly added requests are cancelled
immediately and make it never into request list. Thus, the crash is
avoided, and it is made certain that upon return of ares_cancel(), there
are no requests whatsoever on the channel.
There are two possible contracts I could see for this:
1) ares_cancel() guarantees that all active requests when entering the
function will be cancelled.
2) ares_cancel() guarantees there are no active requests when the
function exits.
The patch clearly implements (2). What are your thoughts on semantics here?
I've never had to use ares_cancel(), so I don't know if it currently states (2).
The ares_cancel() specification makes no explicit statement regarding
callbacks registering new requests. It does use the past tense
("requests made"), but without any indication of whether the point of
reference is function entry or function exit, so any case for either
possibility will remain weak at best.
Personally, I would find it somewhat surprising if I couldn't create
new requests
within an ares_cancel()-executed callback, and would prefer (1).
Funny, for me it's just the other way round.
The best solution in such a case would of course be to support both
behaviours. One could simply pass some kind of flag to ares_cancel(),
but that would break the current API.
How about this:
* Make ares_destroy() more user friendly and allow callbacks to register
new requests on the channel being destroyed. Those requests would then
immediately be finished with ARES_EDESTRUCTION.
* Let ares_cancel() walk through the list, but don't assert it's empty
afterwards.
Then, to get behaviour (1), one would call ares_cancel(), and for
behaviour (2), you would use ares_destroy(), followed by ares_init(). A
little awkward perhaps, but the API would not be broken.
Thoughts?
Best regards,
Alexander
