Subject: Re: [PATCH 1/4] ares_cancel(): ensure cancellation of all requests

Re: [PATCH 1/4] ares_cancel(): ensure cancellation of all requests

From: Alexander Klauer <Alexander.Klauer_at_itwm.fraunhofer.de>
Date: Thu, 21 Mar 2013 13:28:57 +0100

On 03/21/2013 11:45 AM, Tommie Gannert wrote:
> 2013/3/21 Alexander Klauer <Alexander.Klauer_at_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
Received on 2013-03-21