Re: client_side and comm_close

Alex Rousskov wrote:
> On Mon, 2008-04-21 at 15:48 +1200, Amos Jeffries wrote:
>>> 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.
>
> Yes, except if we want to avoid modifying old code that can still access
> those flags directly because it gets immediate close callbacks (more on
> that below).
>
>> (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...
>
> Those are the same close callbacks! There are no low-level and
> high-level close callbacks here. The external code can use its stored
> callback pointer to get FD status even after that close callback has
> been called. There is no problem with that. The callback will not look
> at fd_table in that case, it will just say "yes, the fd is closed as far
> as you should be concerned".
>
> And, per recent suggestions, old code will get immediate close callbacks
> so it does not need to be modified to use the close callback pointer to
> ask about FD status.

Alright. I got to this branch of the thread before the other which makes
things clearer. Same for the below.

>
>>> 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.
>
> Sorry, I do not understand. The "late" registration of a close callback
> can come from anywhere. The old code relies on fd only. There is no way
> for comm to distinguish whether the caller is using a FD from a closed
> connection or a new one. This problem exists in the old code as well so
> there is no new danger here! This problem can only be solved when we
> have comm handlers of one sort or another.
>
>>> 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.
>
> I am confused by the "normal callback that checks the FD is open" part.
> What is that for? Are you talking about some internal comm calls to
> close the FD? I believe that handler code that currently does not ignore
> ERR_COMM_CLOSING notifications will need to be moved into the close
> handler code (because only the close handler will be called if we
> eliminate ERR_COMM_CLOSING).

Nevermind. I was saying it wrong, but meaning what you have re-stated.

Amos

-- 
Please use Squid 2.6.STABLE19 or 3.0.STABLE4