Re: client_side and comm_close

Alex Rousskov wrote:
> On Mon, 2008-04-21 at 16:02 +1200, Amos Jeffries wrote:
>>> On Sun, 2008-04-20 at 22:01 +0300, Tsantilas Christos wrote:
>>>> Maybe it can be easier:
>>> The ease of implementation is a separate question. We still have to
>>> agree what we are implementing, and the items below attempt to define
>>> that. Ambiguity and lack of clear APIs is quite dangerous here (as the
>>> related bugs illustrate!).
>>>
>>>> Just keeping the comm_close as is and in AsyncCallsQueue just
>>>> cancels/not execute asyncCall's for which the fd_table[fd].flags.closing
>>>> is set.
>>> Implementing #1 below can indeed be as simple as you sketch above. We
>>> need to make sure that the final call that closes the FD and makes fd
>>> value available to other connections is placed last (that may already be
>>> true and is easy to fix if not).
>> Not true.
>>
>> IMO, The proposed API _very_ first two lines of code in comm_close are to
>> register a special Comm callback to perform the fclose() call, and then to
>> immediately set fd_table flag closed for the rest of the comm_close
>> process.
>
> Agreed on the flag, disagreed on the call. The special/internal Comm
> call (to self) should be scheduled last (after all close callbacks) and
> not first because the close handler might need access to some FD-related
> info. That info should be preserved until all close handlers have been
> called.
>
>> With that condition at the start we can guarantee that any registrations
>> made during the close sequence are either non-FD-relevant or caught.
>
> Yes, the flag is sufficient for that. The internal "close for good" call
> can still be last.

I was thinking the close-for-good call would get caught as an fd
operation on closed fd by the Async stuff if set after the flag.

>
>> The special Comm callback is only special in that it is not required to
>> check flags open before fclose()'ing the system-level FD, which will allow
>> new connections to open on the FD.
>
> It is special because it is calling an internal comm method not some
> external close handler. Its profile and parameters are different. I
> would not call it a callback because of that, but the label is not
> important. It is not a close callback in terms of
> comm_add_close_handler.

Yet it seems to me it needs to be run as an async after the other async
close-handlers are run. Due to the non-determinence of the async timing.

>> Between the initial comm_close() call and the special Comm callback, we
>> don't need to care if callbacks write their shutdown statements to the fd
>> (its still technically open) but the closed flag prevents comm accepting
>> any new delayed event registrations or reads.
>
> Exactly. Our only problem is with code that calls comm with a stale fd,
> after that fd has been really closed and a new connection was opened
> with the same fd. That's not a new problem and we will solve it in v3.2
> using comm handlers.
>
> I hope the above puts us on the same page about the implementation
> sketch for the comm_close API, but please yell if something still seems
> broken.

Okay. It looks the way I would implement it from scratch.

Amos

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