Re: [PATCH] raw-FD related AsyncCall classes

On 11/23/2011 05:11 AM, Amos Jeffries wrote:
> The problem:
> CommCalls functionality is conflated with calls created to do general
> FD handling (FD as pipe handle, FD as disk handle, FD as pointer into
> the fd_table structure). Sometimes because they do operations mirroring
> comm handlers and also use FD. None of this actually requires the comm
> layer to be involved though.

I do not understand the above. Comm deals with pipe, disk, and socket
communications. Are you assigning some more restricted meaning to "comm
handler" (e.g., comm handler can only deal with communication using
connected sockets)?

> The Comm::Connection objects which comm
> handlers pass around is also very inappropriate for these FD types.

I agree that using Comm::Connection for manipulating non-connections is
wrong. We need a lower level "descriptor" type(s) that non-connection
users and Comm::Connection would use.

> This patch adds functionality for two types of AsyncCall to replace the
> Comm parameters being abused.

I see the "adds" part but I do not see the "replace" part. Which Comm
parameters we will no longer abuse after this patch?

s/two types of AsyncCall/two types of AsyncCall Dialers/?

> * FdeCbParams is added to CommCalls infrastructure, for use internally
> and "lower" than comm API to pass around raw FD values. This should be
> avoided on TCP socket FD, but may be used by callers needing FD where
> Comm::Connection is inappropriate.
>
> * UnaryCbdataDialer<T> template is added to dial a call on any wrapper
> function taking one cbdata state object as a parameter pointer.
> This dialer checks the cbdata base object validity prior to calling. As
> such it acts like the cbdata callbacks, but adding async scheduling
> ability.
> It also adds the AsyncCalls ability for the wrapper functions to have
> typed parameters instead of "void *data" and casting.
>
>
> Alex: I've been searching the Async code for a long while trying to
> figure out how to do this with the existing API. If you can point me at
> how to do it without adding a new template I'd be grateful. Or
> alternatively for any reasons why this does not already exist.

I do not think there is anything wrong with your UnaryCbdataDialer
approach as such. I probably thought that the same energy was better
spent on converting remaining call recipients to Jobs instead. If we go
your route, we will have to change the handlers once again when their
code becomes an AsyncJob, and I do not like changing the same code twice.

> For now this dialer template is isolated in helper.cc and used only for
> the helper pipe() FD close handlers. If others agree I'd like to make it
> globally available in new src/base/AsyncCbdataCalls.h

Yes, please.

> and use it to ease
> the transition from cbdata objects to Jobs. It seems the big hitch
> currently to transitioning is that to use generic Unary/Nullary
> templates the object must be a fully transitioned AsyncJob so we end up
> with custom-coded dialers for each cbdata class type.

I do not see how UnaryCbdataDialer "eases the transition", but it will
improve old-style code so I welcome its addition.

IMO, the "big hitch" is in satisfying AsyncJob requirements. Once that
is done, adjusting callbacks should be very straightforward.

> +/// Special Calls, for direct use of an FD without a controlling Comm::Connection
> +/// This is used for pipe() FD with helpers, and internally by Comm when handling some special FD actions.
> +class FdeCbParams;

Please move class description to the class declaration.

Do we need to predeclare at all? In the patch, I do not see FdeCbParams
(and FDECB) used before they are fully declared.

> +class UnaryCbdataDialer : public CallDialer
> +{
> +public:
> + typedef T Params;
> + typedef void Handler(T *);
> +
> + UnaryCbdataDialer(Handler *aHandler, Params *aParam) :
> + data(cbdataReference(aParam)),
> + handler(aHandler)
> + {}
> +
> + ~UnaryCbdataDialer() { cbdataReferenceDone(data); }
> +
> + bool canDial(AsyncCall &call) { return true; }
> + void dial(AsyncCall &call) {
> + if (cbdataReferenceValid(data))
> + handler(data);
> + }
> +
> + void print(std::ostream &os) const {} // dummy for now.
> +

Please add explicit "virtual" prefix to virtual methods and a
    /* CallDialer API */
comment to explain where they came from.

canDial() should probably return cbdataReferenceValid(data) for call
debugging to work correctly and possibly to reduce call setup overheads.
Similarly, dial() should probably not check cbdataReferenceValid(data)
again because canDial() does.

Please implement the print() method by printing the argument. See
UnaryMemFunT::print(). This is easy (especially after you fix data's
type, see below) and will help with finding the right calls in an ALL,9
trace.

> +template<class T>
> +class UnaryCbdataDialer : public CallDialer
> +{
...
> + Params *data;
> + Handler *handler;

The "data" member should be of type CbcPointer<T>. Besides reusing
existing code, this will simplify your constructor, remove explicit
destructor, and make copying of UnaryCbdataDialer objects work. If you
do not do that, you probably should add private copy constructor and
assignment operator declarations (without implementation).

To preserve symmetry with UnaryMemFunT, please rename T to Data OR
Argument1, removing Params, especially since we only have one parameter
here and not many parameterS.

To avoid the need to typedef dialers manually, consider adding a helper
function. Something along these lines:

template <class Argument1>
UnaryCbdataDialer<Argument1>
cbdataDialer(typename UnaryCbdataDialer<Argument1>::Handler *handler,
             Argument1 arg1)
{
    return UnaryCbdataDialer<Argument1>(handler, arg1);
}

See existing JobMemFun() for similar code and adjust as needed. When you
are done, you should be able to replace:

> + typedef UnaryCbdataDialer<helper_server> SrvDialer;
> + srv->asyncDestructor = asyncCall(5,4, "helperServerFree", SrvDialer(helperServerFree, srv));

with

> + srv->asyncDestructor = asyncCall(5,4, "helperServerFree", cbdataDialer(helperServerFree, srv));

> + /// the AsyncCall which has been created to destruct this object on FD close
> + AsyncCall::Pointer asyncDestructor;

Why do we need to remember this callback? The data member seems to be
unused.

> - int fd; // raw FD which the call was about. use conn instead for new code.
> + int fd; /// FD which the call was about. Set by the caller creator. Use conn instead for new code.

s/caller creator/caller/? The creator of the callback does not set the
fd value.

Also, since not all new code should use Connection after your changes, I
would replace "Use conn instead for new code" with "Use conn if
available" or something like that. I have to say that your changes

> /* CommCalls implement AsyncCall interface for comm_* callbacks.
> * The classes cover two call dialer kinds:
> * - A C-style call using a function pointer (depricated);
> * - A C++-style call to an AsyncJob child.
> * and several comm_* callback kinds:
> * - accept (IOACB)
> * - connect (CNCB)
> * - I/O (IOCB)
> * - timeout (CTCB)
> * - close (CLCB)
> */

This comment in src/CommCalls.h needs to be updated to reflect the
addition of "FD event" callback.

Thank you,

Alex.
Received on Wed Nov 23 2011 - 17:56:54 MST