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).
>
Sounds good.

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

Sound good.

> 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.

Sounds good, BUT, direct access to fd_table pieces may need to be blocked
entirely (private:) so code is forced to go through the Comm API properly.

(2) states that the higher-level close callbacks may be run at any time.
ie after the callback (3) refers to is run. This leaves a window open for
disaster, unless the closing callbacks are made immediate, and back we go
to recursion...

> 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[*].

That non-detection seems to me to be a worry. The same problem in (3)
occurs here. (4) can guarantee that the closing callbacks don't play nasty
re-registrations. But only if they are called immediate instead of
scheduled.

>
> 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?

Apart from the worry with immediate vs delayed closing callbacks.

To reduce that worry somewhat I think, the callbacks which actually use
ERR_COMM_CLOSING for anything other than immediate abort will need to be
replaced with two; a normal callback that checks the FD is open, and a
simple closing callback.

The rest can be left to StackableIO/abstract-handles cleanup. I have some
design ideas there, but thats another topic.

>
> [*] 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.

Amos
Received on Tue Apr 22 2008 - 13:50:36 MDT