Re: Comm API notes

On Thu, 2008-09-11 at 10:27 +0800, Adrian Chadd wrote:
> 2008/9/11 Alex Rousskov <rousskov_at_measurement-factory.com>:
> > * I/O cancellation.
> >
> > To cancel an interest in a read operation, call comm_read_cancel()
> > with an AsyncCall object. This call guarantees that the passed Call
> > will be canceled (see the AsyncCall API for call cancellation
> > definitions and details). Naturally, the code has to store the
> > original read callback Call pointer to use this interface. This call
> > does not guarantee that the read operation has not already happen.
> > This call guarantees that the read operation will not happen.
>
> As I said earlier, you can't guarantee that with asynchronous IO. The
> call may be in progress and not completed. I'm assuming you'd count
> "in progress" as "has already happened" but unlike the latter, you
> can't cancel it at the OS level.

I agree that the last sentence needs to be changed. We can only
guarantee what happens at "squid level" but I lack terminology to
express that nicely.

I may change it to say

        "For a read operation which timing is controlled by Squid, this call
guarantees that the read operation will not happen."

This sounds awkward, but may be accurate. Any better ideas?

> As long as the API keeps all the relevant OS related structures in
> place to allow the IO to complete, and callers to the cancellation
> function are prepared to handle the case where the IO is happening
> versus has already happened, then i'm happy.

Is there Squid3 code that uses asynchronous Comm reads already?

> > You cannot reliably cancel an interest in read operation using the old
> > comm_read_cancel call that uses a function pointer. The handler may
> > get even called after old comm_read_cancel was called. This old API
> > will be removed.
>
> I really did think I had fixed removing the pending callbacks from the
> callback queue when I implemented this. (Ie, I thought I implemented
> enough for the POSIX read/write API but not enough for
> overlapped/POSIX IO.) What were people seeing pre-AsyncCalls?

The pre-AsyncCalls cancellation API was roughly the same as the new
cancellation API. IIRC, there were still race conditions leading to
asserts, but I believe the intent was the same -- to never receive a
read callback. The details were not documented, of course.

> > It is OK to call comm_read_cancel (both old and new) at any time as
> > long as the descriptor has not been closed and there is either no read
> > interest registered or the passed parameters match the registered
> > ones. If the descriptor has been closed, the behavior is undefined.
> > Otherwise, if parameters do not match, you get an assertion.
> >
> > To cancel other operations, close the descriptor with comm_close.
>
> I'm still not happy with comm_close() being used in that way; it seems
> you aren't either and are stipulating new user code aborts jobs via
> alternative paths.

Not sure what you mean. I am fine with comm_close being used to
close/cancel all Comm state for a given descriptor. I do not like it
being used for other purposes, but that use is outside of these Comm
notes scope.

> I'm also not happy with the idea of close handlers to unwind state
> associated with it; how "deep" do close handlers actually get? Would
> we be better off in the long run by stipulating a more rigid shutdown
> process (eg - shutting down a client-side fd would not involve
> comm_close(fd); but ConnStateData::close() which would handle clearing
> the clientHttpRequests and such, then itself + fd?)

I doubt close handlers are needed at all but that API change is only
possible when descriptor-related state is controlled by a single job. A
thorough client_side cleanup would be a good step in that direction.

And yes, a job may have a high-level method for closing a descriptor and
freeing associated job state. Comm would not know anything about it
though so comm_close would still be needed, in some shape or form. It
should be restricted to the descriptor "owner".

Thank you,

Alex.
Received on Thu Sep 11 2008 - 03:57:19 MDT