Re: [PATCH] comm cleanup mk8 - src/comm only

On 24/09/10 04:39, Alex Rousskov wrote:
> On 09/23/2010 09:46 AM, Amos Jeffries wrote:
>> On 23/09/10 04:49, Alex Rousskov wrote:
>>> On 09/22/2010 08:28 AM, Amos Jeffries wrote:
>>>> On 21/09/10 10:00, Alex Rousskov wrote:
>>>>> On 09/19/2010 05:35 AM, Amos Jeffries wrote:
>>>
>>
>> Patch including the changes requested to date attached.
>>
>>
>
>>> AFAIK, comm_close() will remove those handlers automagically because it
>>> will call them with COMM_ERR_CLOSING. However, our code needs to make
>>> sure that it can handle such callbacks until swanSong() is called.
>>
>> K. Thats what I'm slightly worried about. This is inside swanSong. So by
>> the time this set of comm_close scheduled ones occur the swanSong is
>> over and done with. Is the cancel() enough to make dialing drop and skip?
>> connect() which they all lead back to starts with a Must() which will
>> trigger after swanSong() even if the job object itself is still alive.
>>
>> It won't loop more than once, but scheduling is very a sub-optimal way
>> to kill them.
>
> Nothing can happen to the job after swanSong() because the job is
> deleted after it signs its last song. A dead job cannot receive any
> callbacks. You may but do not have to cancel Comm callbacks because they
> will not reach the job anyway. Calling comm_close should be enough to
> keep Comm state consistent.
>
> Specifically:
>
>> + // rollback what we can from the job state
>> + if (conn_ != NULL && conn_->isOpen()) {
>
> .
>
>> + // drop any handlers now to save a lot of cycles later
>> + commSetSelect(conn_->fd, COMM_SELECT_WRITE, NULL, NULL, 0);
>
> _comm_close does that if there is a write callback.
>
>> + commSetTimeout(conn_->fd, -1, NULL, NULL);
>
> _comm_close does that.
>
>> + // it never reached fully open, so abort the FD
>> + conn_->close();
>
> That must be enough.
>
>
>
> The only remaining thing is moving Comm::ConnOpener::connected() to
> Connection::connected() but that is minor. Could be useful for those
> cases were we still connect without using your new code though.
>

Ah thats what you meant. Connection is trying to stay as close to a dumb
storage type as possible.
The lookups fde state changes needed by connected() would make it
heavier. I'd avoid it's close() as well if I could.

NP: long-term the plan is to have TPROXY, TLS/SSL, EUI and IDENT to the
remote end of the new connection also started at the connected() point.
Based on Connection::flags.
  With the equivalent client-facing ones in ConnAcceptor.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.8
   Beta testers wanted for 3.2.0.2