On Wed, 23 Nov 2011 10:56:24 -0700, Alex Rousskov wrote:
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)?
comm as a whole (include fde functionality) does deal with all three.
The comm.cc code though makes a lot of network-centric assumptions about
things that can be done with the FD.
CommCalls.cc functionality in particular is optimized for use by the
comm.cc TCP code, and has almost no relevance to the pipe or disk code.
Yet the pipe and disk callbacks are expected to receive CommCalls
parameters.
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.
From the look I've had so far we don't actually need that.
The pipes are using "void *data" as ahandle to some state object
controllign teh pipe. Disk OPs are still using "int fd" as a handle to
an fde object when they are forced to go through CommCalls. Both the
objects referenced by those alternative handle types maintain their own
state members and flags mostly independent from the comm.cc systems.
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?
Three calls in trunk are "replaced" / altered. They are now using
CommCloseCbParams to pass FD (becomes FdeCbParams) or void*data (becomes
an instance of the CBDATA template Params field)
s/two types of AsyncCall/two types of AsyncCall Dialers/?
One is a new params type, one a new dialer type *and* params type.
Description is a bit fuzzy. I was intending to convey "AsyncCalls" as
feature name more than specific alteration to AsyncCall class.
* 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.
AI agree on the double-change things. I've just been bitten several
times this year needing to add new functionality and not having time to
re-code a whole AsyncJob.
This helper callback is one such case in point. Re-writing the entire
helper IPC API as a Job just to detatch the state destructor from
CommCalls API dependency would easily blow out into a massive effort and
code change.
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.
I was assuming that since AsyncJob is cbdata child and does cbdata
validation prior to dialing the job members, that mirroring that check
would be sufficient for other cbdata.
- validate is performed before dialing the call and cbdata done() is
used on destructing the call (dialed or not).
The other core requirement that the async call handler has no
expectation of timing, only of sequence. Seems to be met by the cbdata
handlers assumption that the calling code must exit immediately, or move
on to independent code operations not trusting the passed cbdata object
or parameters.
Have I missed anything?
+/// 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.
Done.
Do we need to predeclare at all? In the patch, I do not see
FdeCbParams
(and FDECB) used before they are fully declared.
I put the typedef there to match the other typedefs (I assume you did
that to make the API handler types easily found). At this stage I'm
unsure how many of the pipe() and disk I/O handlers will need to use
this new CommCall parameter object. Only the accept() and close()
handlers have been re-checked so far. I hope to remove it from the comm
API into an internal type, but won't know for sure until the handler
audit is finished.
Some compilers require declaration before typedef to avoid incomplete
or undefined type warnings. This pre-declare seems to silence them fine.
+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.
Hm. Okay. Was trying to avoid complex/slow canDial(), but agreed. done.
I was not entirely clear what happens to the scheduled call if
!canDial() code path. Drop the queue refcount and destruct?
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.
Done.
+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).
Done.
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.
Done.
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);
}
Done.
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));
Done.
+ /// 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.
I was thinking explicit cancel. But it seems not necessary in the end.
Dropped.
- 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.
The current ones using this do. See _comm_close() in comm.cc.
I would like the commCallCloseHandlers() not care about duplicating
that effort. Just to schedule them all and finish.
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
?? missing text?
/* 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.
Done.
Thank you. Build testing that all now. Updated patch to follow.
Amos
