Re: [PATCH] bug 3081: comm layer cleanups for TCP acceptor
On 01/22/2011 06:46 AM, Amos Jeffries wrote: 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. Thank you for polishing this! I do not see any more problems. This is minor, but you do not need to use the inline keyword for methods that are immediately inlined, such as CommCbFunPtrCallT copy constructor. Cheers, Alex.
Re: [PATCH] bug 3081: comm layer cleanups for TCP acceptor
On Tue, 25 Jan 2011 17:42:03 -0700, Alex Rousskov rouss...@measurement-factory.com wrote: On 01/22/2011 06:46 AM, Amos Jeffries wrote: 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. Thank you for polishing this! I do not see any more problems. This is minor, but you do not need to use the inline keyword for methods that are immediately inlined, such as CommCbFunPtrCallT copy constructor. Cheers, Alex. Wonderful. Thank you so much for the time you have been putting into the audits. Will fix that inline and commit in a few hours. Amos
Re: [PATCH] bug 3081: comm layer cleanups for TCP acceptor
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 CommAcceptCbPtrFuno);
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.
inline CommCbFunPtrCallT(int debugSection, int debugLevel,
const char *callName, const
DialeraDialer);
+inline CommCbFunPtrCallT(const Pointerp) :
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 CommCbFunPtrCallTCommAcceptCbPtrFun AcceptCall;
RefCountAcceptCall subCall = commCbCall(5, 5, httpAccept,
CommAcceptCbPtrFun(httpAccept, s));
Subscription::Pointer sub = new CallSubscriptionAcceptCall(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
‘RefCountAsyncCall CallSubscriptionCall_::callback() const [with
Call_ = CommCbFunPtrCallTCommAcceptCbPtrFun]’:
../../trunk/src/client_side.cc:4092: instantiated from here
../../trunk/src/base/Subscription.h:45: error: no matching function for
call to ‘CommCbFunPtrCallTCommAcceptCbPtrFun::CommCbFunPtrCallT(const
RefCountCommCbFunPtrCallTCommAcceptCbPtrFun )’
../../trunk/src/CommCalls.h:302: note: candidates are:
CommCbFunPtrCallTDialer::CommCbFunPtrCallT(int, int, const char*,
const Dialer) [with Dialer = CommAcceptCbPtrFun]
../../trunk/src/CommCalls.h:263: note:
CommCbFunPtrCallTCommAcceptCbPtrFun::CommCbFunPtrCallT(const
CommCbFunPtrCallTCommAcceptCbPtrFun)
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 CommCbFunPtrCallTCommAcceptCbPtrFun.
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 +
+++ src/base/Subscription.h 2011-01-21 14:42:52 +
@@ -42,7 +42,7 @@
public:
/// Must be passed an object. nil pointers are not permitted.
explicit CallSubscription(const RefCountCall_ 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 RefCountCall_ 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).
+AsyncCall();
+AsyncCall(const AsyncCall);
I could not find the implementation for these two, but I probably just
missed it. I assume the default/generated constructors did not work.
Where do we use these two explicit ones?
We don't. These were what you asked me to add when we
Re: [PATCH] bug 3081: comm layer cleanups for TCP acceptor
On 01/21/2011 08:55 AM, Amos Jeffries wrote:
Hmmm, bug in Subscription then.
- virtual AsyncCall::Pointer callback() const { return new Call_(call); }
+ virtual AsyncCall::Pointer callback() const { return new
Call_(call.getRaw()); }
private:
const RefCountCall_ call; /// gets copied to create callback calls
Still does not look right. getRaw() returns a pointer to the call. You
do not want to copy a pointer. You want to copy the call itself:
virtual AsyncCall::Pointer callback() const { return new Call_(*call); }
And if Call_(call.getRaw()) actually compiles, there is another bug
somewhere, allowing a pointer-to-AsyncCall conversion to AsyncCall.
HTH,
Alex.
Re: [PATCH] bug 3081: comm layer cleanups for TCP acceptor
On 01/21/2011 08:55 AM, Amos Jeffries wrote: 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. There are several problems here adding to my confusion: * The lack of method definition (a.k.a. body or implementation) cannot prevent anything at compile time -- the method definition might be in some library. The compiler does not know this until it tries to link an executable. Thus, definition is a red herring here, and you can ignore it for the purpose of this discussion. * Terminology: - Default constructor for class C has no parameters: C(). - Copy constructor for class C has one specific parameter: C(const C). - Implicit constructor is the one generated by the compiler if the class declaration does not provide an explicit one. Compiler can generate implicit default constructor, implicit copy constructor, implicit assignment operator, and implicit destructor. * All of these implicit members are always generated if not explicitly provided, except for the default constructor: An implicit default constructor is generated only if there are no explicit constructors of any kind. This makes sense: if we said how to create an object of a class, the compiler must not provide its own creation method as an alternative! * A parent copy constructor cannot block child's default constructor. The two are totally independent: copying and creation are different things. * Similarly, a parent default constructor cannot block child's copy constructor. HTH, Alex.
Re: [PATCH] bug 3081: comm layer cleanups for TCP acceptor
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 CommAcceptCbPtrFuno);
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.
inline CommCbFunPtrCallT(int debugSection, int debugLevel,
const char *callName, const
DialeraDialer);
+inline CommCbFunPtrCallT(const Pointerp) :
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 CommCbFunPtrCallTCommAcceptCbPtrFun AcceptCall;
RefCountAcceptCall subCall = commCbCall(5, 5, httpAccept,
CommAcceptCbPtrFun(httpAccept, s));
Subscription::Pointer sub = new CallSubscriptionAcceptCall(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
‘RefCountAsyncCall CallSubscriptionCall_::callback() const [with
Call_ = CommCbFunPtrCallTCommAcceptCbPtrFun]’:
../../trunk/src/client_side.cc:4092: instantiated from here
../../trunk/src/base/Subscription.h:45: error: no matching function for
call to ‘CommCbFunPtrCallTCommAcceptCbPtrFun::CommCbFunPtrCallT(const
RefCountCommCbFunPtrCallTCommAcceptCbPtrFun )’
../../trunk/src/CommCalls.h:302: note: candidates are:
CommCbFunPtrCallTDialer::CommCbFunPtrCallT(int, int, const char*,
const Dialer) [with Dialer = CommAcceptCbPtrFun]
../../trunk/src/CommCalls.h:263: note:
CommCbFunPtrCallTCommAcceptCbPtrFun::CommCbFunPtrCallT(const
CommCbFunPtrCallTCommAcceptCbPtrFun)
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 CommCbFunPtrCallTCommAcceptCbPtrFun.
Note that fixing this may expose other problems.
+AsyncCall();
+AsyncCall(const AsyncCall);
I could not find the implementation for these two, but I probably just
missed it. I assume the default/generated constructors did not work.
Where do we use these two explicit ones?
We don't. These were what you asked me to add when we were creating
Subscription, to prevent default constructors being (ab)used on any old
call. This non-definition enforces the AsyncCall and children are not
copied around and double-scheduled.
Ah, please move these two declarations to the private: section and add
a comment saying that they are not implemented to prevent nil calls from
being passed around and unknowingly scheduled, for now.
+AsyncCallT(const RefCountAsyncCallTDialero):
+AsyncCall(o-debugSection, o-debugLevel, o-name),
+dialer(o-dialer) {}
+
Similar to the earlier comments, it seems wrong to provide a
pointer-to-T to T conversion, especially an implicit one where the
default copy constructor should work fine.
Parallel of CommCbFunPtrCallT(const Pointerp). This is required for the
matching case in FTP which uses AsyncCallT for a generic Uniary member
callback.
Same comment. A different solution is needed here unless I am missing
something. We should not add pointer-to-T to T conversions.
if (flag != COMM_OK)
Re: [PATCH] bug 3081: comm layer cleanups for TCP acceptor
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.
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?
+AsyncCall();
+AsyncCall(const AsyncCall );
I could not find the implementation for these two, but I probably just
missed it. I assume the default/generated constructors did not work.
Where do we use these two explicit ones?
+AsyncCallT(const RefCountAsyncCallTDialer o):
+AsyncCall(o-debugSection, o-debugLevel, o-name),
+dialer(o-dialer) {}
+
Similar to the earlier comments, it seems wrong to provide a
pointer-to-T to T conversion, especially an implicit one where the
default copy constructor should work fine.
if (flag != COMM_OK) {
-debugs(33, 1, httpAccept: FD sock : accept failure:
xstrerr(xerrno));
+// This should not occur with TcpAcceptor.
+// However its possible the call was still queued when the client
disconnected
+debugs(33, 1, httpAccept: FD s-listenFd : accept failure:
xstrerr(xerrno));
This new comment confuses me. Is it possible or not? Sounds like it is
possible and is not a bug. Why complain at level 1 then? Overall,
httpAccept() and httpsAccept() changes seem either unnecessary or out of
the patch scope.
+debugs(5, 5, HERE FD fd , local_addr AsyncCall
Subscription: aSub);
Consider moving the part after HERE to TcpAcceptor::status() method so
that you do not have to repeat it. It will be used whenever the acceptor
is called asynchronously (and you can add explicit debugs calls too, of
course).
+} catch(const TextException e) {
+fatalf(FATAL: error while accepting new client connection: %s\n,
e.message);
It is better to catch std::expression and print its message using
e.what(). This way, you will catch and report details of both standard
exceptions and our TextExceptions because TextException is a child of
std::exception.
+comm_err_t status = oldAccept(newConnDetails);
Avoid status because AsyncJob has a virtual method called status().
Consider err or result.
Make it const unless we do change it.
+/// temporary holder for newely accepted client FD
+int newFd_;
...
+// drop the temporary recent accepted socket FD details.
+// this prevents information crossover on calls.
+newFd_ = -1;
Why not keep it as a local variable and pass it to notify(), like the
old code did? That feels like a simpler/safer approach to me than
storing this temporary information in a data member.
+/** Subscribe a handler to receive calls back about new connections.
+ * Replaces any existing subscribed handler.
+ */
+void subscribe(const Subscription::Pointer aSub);
Consider s/Replaces/Unsubscribes/ to indirectly document that pending
calls remain scheduled.
+ * Pending calls will remain scheduled.
Consider Already scheduled callbacks remain scheduled.
+private:
+friend class AcceptLimiter;
This looks weird because friends cannot access private members. They can
only access protected ones and you do not have any protected members.
Either this friendship is not needed at all or you can move some public
members to become protected.
+Comm::TcpAcceptor *tmp = new Comm::TcpAcceptor(fd, data.local,
data.flags, note, sub);
+
+// Ensure we have a copy of the FD opened for listening and a close
handler on it.
+assert(data.fd == -1 || data.fd == tmp-fd);
+data.fd = -1;
+data.opened(tmp-fd, dataCloser());
+
+AsyncJob::Start(tmp);
This sequence is leak-prone. Either check everything
