Re: [PATCH 1/4] ares_cancel(): ensure cancellation of all requests
On Thu, 21 Mar 2013, Alexander Klauer wrote: Implementation-wise, I think I would just swap the list head in the ares channel with an empty list head on the stack, and then start walking through the list with the list head on the stack. Any new requests would then be added to the now initially empty list in the ares channel. I think this sounds like a really sane approach that is uncomplicated, gives no surprises to a user and is easy to document as well. -- / daniel.haxx.se
Re: [PATCH 1/4] ares_cancel(): ensure cancellation of all requests
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). Personally, I would find it somewhat surprising if I couldn't create new requests within an ares_cancel()-executed callback, and would prefer (1). -- Tommie
Re: [PATCH 1/4] ares_cancel(): ensure cancellation of all requests
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
Re: [PATCH 1/4] ares_cancel(): ensure cancellation of all requests
2013/3/21 Alexander Klauer alexander.kla...@itwm.fraunhofer.de: What happens if someone calls ares_cancel() while ares_cancel() is in progress? With (1), that would mean that the topmost (most recent) instance ares_cancel() has to cancel all those requests that appeared since the next-to-topmost instance was invoked, right? Good corner case. I try to think of how I would expect it to work if it was just multiple threads doing requests and ares_cancel() would cancel them. And I think what you suggest makes a lot of sense. The second call would be responsible for cancelling all active requests though, so if you have three requests, A B C, and cancelling A causes a request D to be created, and then B calls cancel(), then you would have a request list like C D when entering that second cancel(). In that case both the first and second cancel() would be responsible for cancelling C, and it would have to be cancelled before the second cancel() returns (and implicitly thus also the first as they were recursive calls.) Might be tricky to get right without ref counting. If it requires a lot of code changes, maybe it's not worth enforcing such strict semantics, though. No real use case, my leaning towards (2) is just personal feeling. My actual use case right now is to call ares_cancel() immediately prior to ares_destroy(). With the implementation of (1), I would of course simply call ares_destroy(). I also call ares_cancel() in an out of memory condition. Then it sounds like the proposed functionality would actually benefit your code? Come to think of it, ares_cancel() is pretty useless. Normally, you would either want to cancel one single request, the functionality for which is missing, or cancel everything when you destroy the channel. Yupp, I know. :) It was one of the biggest surprises when I started using it. *Thinking out loud* Could one perhaps add a channel flag that would alter I think you mean the whole channel, not the whole library. Ah, yes. I'm not sure that's a good idea. All the other public flags are about what ares does with stuff sent and received over the network, not internal behaviour. shutdown() has a specific use in the case of TCP connections (and it does get its own flags). OTOH, my current implementation of (2) uses just that flag variable internally. Aight, let's scrap that line of thought. -- Tommie
