Re: [PATCH] bug 3081: comm layer cleanups for TCP acceptor

On 22/01/11 04:55, Amos Jeffries wrote:
> On 20/01/11 11:50, Alex Rousskov wrote:
>> On 01/17/2011 10:03 PM, Amos Jeffries wrote:
>>> On 18/01/11 10:08, Alex Rousskov wrote:
>>>> On 01/14/2011 09:11 PM, Amos Jeffries wrote:
>>>>> On 10/01/11 14:02, Amos Jeffries wrote:
>>>>>> Attached patch contains the cleanup-comm branch changes to
>>>>>> ListenStateData which make it an AsyncJob (called Comm::TcpAcceptor)
>>>>>> using Subscriptions to generate calls.
>>>>>>
>>>>>> This fixes the call re-use problem of bug 3081 and a related FTP data
>>>>>> connection bug.
>>>>>>
>>>>>> Alex:
>>>>>> please test that this does not break with SMP support. I've run it
>>>>>> with
>>>>>> 2 workers on a non-SMP machine, but have not checked for anything
>>>>>> more
>>>>>> complex.
>>>>>>
>>>>>
>>>>> Have you had any time to double-check me on this Alex?
>>>>>
>>>>> I don't want to rely on the silence clause for merging this one.
>>>>
>>>> Sorry I could not review this earlier.
>>>>
>>>>
>>>>> NP: attached updated version against trunk r11159.
>>>>
>>>>> + CommAcceptCbPtrFun(const CommAcceptCbPtrFun&o);
>>>>
>>>> Are you sure this is needed? It seems like its implementation is what
>>>> the default copy constructor does. If there is a reason to add it, you
>>>> should add a destructor as well, but I think everything should work
>>>> without these additional explicit members.
>>>
>>> Since we have AsyncCall blocking the default constructor from allowing
>>> null calls to be passed around we need to add an explicit constructor in
>>> the particular children which subscription requires copying for.
>>
>>
>> This does not compute for me. A default (parameter-less) constructor,
>> blocked or not, should not affect copy constructors. Can you rephrase
>> what you are trying to avoid or achieve? Or perhaps quote the errors you
>> get when the above explicit copy constructor is removed.
>>
>
> I was thinking AsyncCall(const AsyncCall &); with no definition blocked
> the children default constructors too. Due to the parent not having one
> available to call from the default child constructor.
>
> Doing a test build now to verify. If it passes will drop.
>

Dropped CommAcceptCbPtrFun.

>>>>> inline CommCbFunPtrCallT(int debugSection, int debugLevel,
>>>>> const char *callName, const
>>>>> Dialer&aDialer);
>>>>>
>>>>> + inline CommCbFunPtrCallT(const Pointer&p) :
>>>>
>>>> This additions seems wrong on two counts:
>>>>
>>>> a) You should not add a conversion from a pointer to CommCbFunPtrCallT
>>>> to CommCbFunPtrCallT itself.
>>>>
>>>> b) You could add an explicit copy constructor, but the default copy
>>>> constructor should work for copying CommCbFunPtrCallT objects already,
>>>> no?
>>>
>>> Without them ... this:
>>>
>>> // setup the subscriptions such that new connections accepted by
>>> listenConn are handled by HTTP
>>> typedef CommCbFunPtrCallT<CommAcceptCbPtrFun> AcceptCall;
>>> RefCount<AcceptCall> subCall = commCbCall(5, 5, "httpAccept",
>>> CommAcceptCbPtrFun(httpAccept, s));
>>> Subscription::Pointer sub = new CallSubscription<AcceptCall>(subCall);
>>>
>>>
>>> Generates this:
>>>
>>> In file included from ../../trunk/src/comm/TcpAcceptor.h:6,
>>> from ../../trunk/src/client_side.cc:103:
>>> ../../trunk/src/base/Subscription.h: In member function
>>> ‘RefCount<AsyncCall> CallSubscription<Call_>::callback() const [with
>>> Call_ = CommCbFunPtrCallT<CommAcceptCbPtrFun>]’:
>>> ../../trunk/src/client_side.cc:4092: instantiated from here
>>> ../../trunk/src/base/Subscription.h:45: error: no matching function for
>>> call to ‘CommCbFunPtrCallT<CommAcceptCbPtrFun>::CommCbFunPtrCallT(const
>>> RefCount<CommCbFunPtrCallT<CommAcceptCbPtrFun> >&)’
>>> ../../trunk/src/CommCalls.h:302: note: candidates are:
>>> CommCbFunPtrCallT<Dialer>::CommCbFunPtrCallT(int, int, const char*,
>>> const Dialer&) [with Dialer = CommAcceptCbPtrFun]
>>> ../../trunk/src/CommCalls.h:263: note:
>>> CommCbFunPtrCallT<CommAcceptCbPtrFun>::CommCbFunPtrCallT(const
>>> CommCbFunPtrCallT<CommAcceptCbPtrFun>&)
>>
>> If I parse this correctly, you are, essentially, passing a pointer-to-X
>> where reference-to-X is expected. The solution is _not_ to add a
>> conversion from pointer-to-X to X, but to pass the X object itself.
>>
>> In your case, X is CommCbFunPtrCallT<CommAcceptCbPtrFun>.
>>
>> Note that fixing this may expose other problems.
>>
>
> Hmmm, bug in Subscription then.
>
> === modified file 'src/base/Subscription.h'
> --- src/base/Subscription.h 2010-10-07 07:53:45 +0000
> +++ src/base/Subscription.h 2011-01-21 14:42:52 +0000
> @@ -42,7 +42,7 @@
> public:
> /// Must be passed an object. nil pointers are not permitted.
> explicit CallSubscription(const RefCount<Call_> &aCall) : call(aCall) {
> assert(aCall != NULL); }
> - virtual AsyncCall::Pointer callback() const { return new Call_(call); }
> + virtual AsyncCall::Pointer callback() const { return new
> Call_(call.getRaw()); }
>
> private:
> const RefCount<Call_> call; ///< gets copied to create callback calls
>
>
> Build testing this now. With the two related constructor pointer-to
> changes. Should have a new patch for you tomorrow (mutual time).
>

Fixing the getRaw() bug this change to Subscription works.

However we cannot drop the CommCbFunPtrCallT and AsyncCallT copy
constructors. Notice how they explicitly call AsyncCall(a,b,c) instead
of the AsyncCall copy contructor. Without this we loose the call
debugs() and the ID creation.

Adding the assignment operator (undefined) and destructor (no-op) to
match our big-three requirements.

New version attached. This clears up all the points we have been talking
about. As well as several duplicate calls to switchTimeoutToData() in FTP.

Amos