On Thu, 2008-04-10 at 23:55 +0200, Henrik Nordstrom wrote:
<snip>
Please allow me to skip most of the "general" questions. I believe your
"blurry lines" argument is right on the mark here, and we will just
confuse each other more unless we start applying general rules to
specific examples.
> > * Cancellation by the recipient has immediate effect (i.e., the call
> > will not be dialed after cancel() is called), but may be "too late" if
> > the call has already been dialed (i.e., it has happened or is
> > happening).
>
> If it has already happened there is nothing to cancel.
>
> Not sure what you refer to by "happening". We are a single thread and a
> thing has either happened or has not happened yet.
Receiving or handling a call is not an instantaneous event though (and
never was). Consider this pseudo-code:
Call::dial() {
// not dialed yet
foo.handleThisCall(); // dialing or "happening"
// dialed or "happened"
}
Foo::handleThisCall() {
// still dialing or "happening"
... if some code here tries cancel this call, it would be a noop
// still dialing or "happening"
}
This may remind you of the old recursive callback mess because the "..."
code can access the call being dialed. I doubt we can completely avoid
this without complicating the API a lot and I think there is little
danger here because the problem is isolated to calls and calls are
mostly dumb parameter boxes that should not create recursion loops.
> > You are doing the right thing, but I think you need to look at the
> > higher level. Async queue does not exist as far as the creator, the
> > caller, and the recipient are concerned. From their point of view, async
> > queue is just a random time delay, nothing else. See the cancellation
> > rules above.
>
> So when is one supposed to use cancellation?
When one does not want to receive the call back.
In other words, only call recipients should cancel calls. What makes it
confusing is that the current code uses things like comm_read_cancel
which can be interpreted as the recipient canceling its callback but
might be called by non-recipients as well. The suggestions below try to
clear this mess.
> To focus lets look at comm_read / comm_read_cancel in isolation. What
> would the purpose of cancelling the call be there? Is there any use of
> AsyncCall::cancel there, or should it just remember that the comm_read
> operation has been cancelled and completely forget about the call?
>
> Or should be the caller of comm_read which remembers that it cancelled
> the read?
>
> Or should it be the caller of comm_read which passes in the Call, and
> also gives this to comm_read_cancel?
Here is what I would suggest (specific application to comm_read is right
below these three general rules!):
* Callbacks must be called: If Foo gets a call pointer as a callback,
Foo must make that call (possibly with a Foo-specific error indicator).
Whether that call will be dialed is irrelevant here.
* Unwanted calls must be canceled: If Bar gives Foo a call pointer as a
callback, but may not be able to handle the call back later, then Bar
must keep a pointer to the same call and cancel that call when needed.
Whether that call will be scheduled (by Foo) is irrelevant here.
* Processed calls must be forgotten: If you scheduled, received, or
cancel the call, set its pointer to NULL. Foo above must forget the call
after scheduling it. Bar above must forget the call when it receives or
cancels it. These are just cleanup rules to avoid double-scheduling or
double-cancellation. Mistakes here are not fatal.
Let's apply these rules to your specific comm_read/comm_read_cancel
example (please read all three paragraphs below before complaining :-):
Comm_read accepts and stores a callback pointer. Thus, it promises to
make that call no matter what. If things go out of hand with the file
descriptor, comm will make that call with a comm error flag set in the
parameters.
If the reader does not want to or cannot read any more, it cancels the
call. It needs to keep a pointer to the call if it might cancel the
call. If the same reader thinks that comm might still be reading from
that file descriptor, the reader also calls comm_read_cancel. This last
sentence is unrelated to calls; it just makes sure comm is not doing
something in vein. Comm_read_cancel cancels the read, not the callback!
Call:cancel cancels the call, not the read.
You may have noticed that in most cases, calling comm_read_cancel would
result in comm scheduling the call that will _not_ be dialed (or even
queued) because the reader has canceled it. This does not lead to bugs,
but wastes a few cycles. There are ways to avoid that waste:
If and only if we are certain that comm_read_cancel is always called by
the reader, then we can eliminate that waste by giving comm_read_cancel
a pointer to the-call-pointer parameter. That adjusted comm_read_cancel
can both cancel/NULL the call and stop I/O monitoring. We can also
rename it to something like comm_cancel_my_read. That adjusted function
will sit partially in reader's space (canceling reader's call) and
partially in comm's space (canceling I/O). The line will be blurry, but
we will save a few cycles.
We can also make comm check whether the callback is canceled before
performing costly scheduling operations (if any). This trick will work
only if the reader cancels the callback before calling comm_read_cancel
so it is not reliable.
Now, let's see what comm_close do. Just like comm_read_cancel, it must
call all registered callbacks with ERR_CLOSING or some such. Again, raw
comm_close is not (or should not be) about callbacks; it is about I/O.
And again, if we want to optimize, we could add comm_close_my_io that
will cancel the callbacks first.
Does this answer your questions?
Thank you for working on this,
Alex.
