Re: client_side and comm_close

On Sun, 2008-04-20 at 09:22 +0200, Henrik Nordstrom wrote:
> lör 2008-04-19 klockan 23:05 -0600 skrev Alex Rousskov:
> > It is easy to implement two kinds of call scheduling, but I am not sure
> > I understand the merits of doing so. What do you mean by the "event
> > loops" in this context? Are you talking about eventAdd() queue? Or I/O
> > event (i.e., comm) callbacks? I am guessing it is the latter.
>
> Both. The event checks (I/O, timed events etc) run from the main loop
> and are always the first AsyncCall callers for anything going on.
>
> > The immediate calls from comm were not really "safe" (because of the
> > recursive/reentrant behavior) so I am not sure what we would achieve
> > here.
>
> As everything going on in Squid is initiated by the event loop you don't
> get recursion from this change..
>
> main loop -> event loop -> immediate async call -> handler -> other I/O
> actions etc but from here always async..

I agree regarding the first initiation point, but disagree that it is
sufficient to guarantee non-recursive behavior: In addition to
callbacks, we have direct function calls to comm and other APIs. The
combination of immediate callbacks and those function calls will cause
"recursion" as control will reenter the jobs multiple times. For
example:

event loop -> immediate callback -> handler -> comm_* call -> immediate
callback -> handler -> ...

> > Please clarify which of the following comm_close implementation would
> > solve the problems you want to solve in this context:
> >
> > A) Immediately call all not-yet-dialed callbacks for the affected FD.
> > All relevant I/O callbacks are called with ERR_COMM_CLOSING flag set (if
> > we still have that). The closing callbacks, if any, are called last.
> >
> > B) Asynchronously call all not-yet-dialed callbacks for the affected FD.
> > All relevant I/O callbacks are called with ERR_COMM_CLOSING flag set (if
> > we still have that). The closing callbacks, if any, are called last.
> >
> > C) Other.
>
> Don't call them. See ERR_COMM_CLOSING discussion.
>
> The I/O calls are no longer relevant at the moment the fd has been
> semantically closed (comm_close called by the code). Only the close
> callbacks matter at that time.
>
> Note: The close callbacks as such need to be async, just not the closing
> of the fd (or at least at a semantical level).

OK, I think you picked option B then :-). Let me polish it to better
match the current level of understanding and the "dropping
ERR_COMM_CLOSING" agreement:

comm_close(fd) API:

1) No I/O callbacks will be dialed after comm_close is called (including
the time when comm_close is running).

2) All close callbacks registered before comm_close was called will be
called asynchronously, sometime after comm_close exits.

3) The code can ask Comm whether a FD associated with a close callback
has been closed. The answer will be "yes" after comm_close is called and
"no" before that. This interface needs to be added. Direct access to
fd_table[fd].flags will not work because the same FD could have been
assigned to another connection already. The code will have to use its
close callback to determine FD status.

4) Registering any callback after comm_close has been called is wrong.
Comm will assert if it can detect such late registrations. Not all late
registrations will be detected because the same FD can be already
assigned to a different (new) connection[*].

The above comm_close API is easy to implement without massive code
changes. Do you think it is the right API? If not, what should be
changed?

[*] We can reliably detect #4 violations by replacing "fd" integers
outside of comm with a unique integer ID or, better, a handler object
containing metadata. That kind of change may be too big for v3.1 though.

Thank you,

Alex.
Received on Tue Apr 22 2008 - 13:48:49 MDT