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:
* I do not like how Comm::ConnOpener::connect() calls itself. I assume
you did not invent this, but the code will fail to produce the
intended(?) delay if there are no other async calls already scheduled,
which is something the class does not really control. Recall that async
calls are AsyncCallQueue::fire()d until there are none left...
At the very least, please add a comment explaining why we use such a
hack instead of either using an explicit loop or waiting a little bit
via eventAdd.
I think you asked me to remove eventAdd a while back before this was a
Job. Reverted to the old .05 sec delay.
I could have made the wrong call, you could have interpreted my comment
incorrectly, or both. If you want to review what happened, please find
and repost that conversation.
* 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.
Considering no other jobs etc have anything validly pending on this
brand new FD should I be calling fd_close(conn_->fd) here instead of
conn_->close()?
In general, if you used comm_open, use comm_close.
If comm_close works, let's use it, avoiding low-level calls we should
not even have access to. Comm itself may have something pending on that
new FD...
* 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.
* cbdataReferenceDone() already sets its parameter to NULL. No need to
do that twice:
+ /* clear any previous ptr */
+ if (_peer) {
+ cbdataReferenceDone(_peer);
+ _peer = NULL;
+ }
Great.
At this random point we are not sure its valid. For now I've make the
if() use the fixed getPeer().
That would leak _peer if it became invalid (unless you call
cbdataReferenceDone(_peer) in getPeer() now, which would be technically
correct and efficient but weird).
Would it be correct to use
cbdataReferenceValidDone(NULL, _peer) unconditionally here with no
callback? instead of separate validate and done?
You do not care about _peer validity or value because you are not
dereferencing that pointer. Just do:
cbdataReferenceDone(_peer);
No ifs, no setting to NULL, no getPeer().
* 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.
* 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.
Cheers,
Alex.
