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

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.

>
>>> * I may be wrong, but the interesting "Is opened conn fully open?"
>>> comment below may reveal a couple of bugs:
>>>
>>>> + // recover what we can from the job
>>>> + if (conn_ != NULL && conn_->isOpen()) {
>>>> + // it never reached fully open, so abort the FD
>>>> + conn_->close();
>>>> + ...
>>>
>>> Clearly, conn_ is open inside that if-statement (there is no such thing
>>> as half-open conn, I hope; we have enough headaches from half-closed
>>
>> sorry, no such luck. see COMM_INPROGRESS.
>
> I see. Fortunately, my swanSong() fix still applied.
>
>> I also see that I missed clearing the write handler before fully closing
>> conn_. do the timeout and closing handlers need to be explicitly
>> removed? or are they happy with the cancels?
>
> 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.

>>> * Can you explain how "syncWithComm() side effects hit
>>> theCallSub->callback()"? Is not theCallSub->callback() itself constant
>>> and its result is stored in a local variable? I do not understand how
>>> syncWithComm() can affect theCallSub itself. Thank you.
>>>
>>
>> Possibly obsolete now.
>>
>> Dialer copy-constructors was initially created using getDialer() which
>> call syncWithComm() which in CommIoCb child class updates this->* state.
>> Fallout of that is that:
>> the CommIoCb constructor cant take a const parameter,
>> the parent class cant provide a const overload constructor,
>> the other children cant either,
>> callback() using copy-constructor cant be const.
>>
>> I *think* the copy constructors are okay using raw dialer pointers now.
>> Which means everything down the chain can be const.
>
>
> I cannot comment on that without seeing the new call-copying code.
>

K. coming next.

>>> * Consider s/_peer/peer_/ in Connection for consistency with most other
>>> code.
>>
>> The "struct peer_" global symbol clashes and gcc complains about missing
>> ; or = after variable "struct".
>> I don't plan on fighting that one just yet. Will mark it to be done
>> though.
>
> Agreed. The struct should be renamed (to Peer?), but that is not your
> problem. You can use just "peer" for the member name, I guess.

which is the global typedef with same clash. :(

>
>
>>> * s/unused//g. C++ does not require naming unused parameters.
>>
>> Done. And converted the two places (commSetSelect and eventAdd wrappers)
>> to use JobCallback with NullaryMemFunT dialers on connect().
>
> Not sure what you mean. Will become clear when the code is posted, I guess.
>

Amos

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