[Xorp-hackers] [PATCH] Supporting asynchronous method implementations

[Steven Simpson]

Hi Ben,

On 11/03/11 14:59, Ben Greear wrote:
> This looks a lot better.  There are a few things that bother me,
> but not sure there is a better way to do it:
>
> * I don't like the typedef'd return values that change based on
> enabling/disabling the async-server.

Yes, it bothered me that it might be hard to follow, but at least it
doesn't spill out into the process-writer's domain, and it rather neatly
(I think) avoids maintenance of two /almost/ identical lines or blocks
in a few cases.

> Maybe we can use a helper method somehow?

Not sure what you mean here, but I fear that anything else would just
degenerate into awkard #ifdefs and almost duplicate maintenance.  I'm
open to suggestions, though.

>   I don't mind if enabling async-server adds a small bit
> of code/size to the binaries, so not everything has to be #ifdef'd
> out, for instance.

I didn't see many opportunities for keeping things in regardless of the
sync-async choice, that I haven't taken already.

> * The Macro that returns one type or another should be all caps
>  at the least.  It will be confusing either way, but at least all-caps
>  gives the reader a clue it's a macro instead of a standard function.

Okay - there are three macros:

    * XrlCmdReturnError
    * XrlCmdOptCallback
    * XrlDispatcherReturnError

How exactly should they be capitalised?  Will XRL_CMD_RETURN_ERROR do,
for example?  Or XORP_something?

> Have you run 'scons check' with and without this enabled?

I have now.  Diffing isn't terribly helpful - timestamps and process ids
vary - but I see from a partial scan that most lines have a counterpart
in each file, with a little re-ordering.  Just grepping for "Tests"
showed the same successes and failures in both cases - I hope those are
the most important lines.

There are certain aspects of the asynchronous stuff that won't be being
tested atm.  I managed to find a place in xorp/libxipc/xrl_pf_stcp.cc,
in XrlPFSTCPSender::read_event, which surely won't work if two replies
from the same server to the same client return out of order:

    if (sph.seqno() != _requests_sent.front()->seqno()) {
	die("Bad sequence number");
	return;
    }

_requests_sent is a list<ref_ptr<RequestState>>.  Requests with
increasing seqnos are pushed to the back, and only the front is
consulted when a response comes in - so it is assuming that responses
arrive in request order.  The code doesn't fail at the moment, since
(almost) all XRLs are implemented synchronously anyway, and so a process
can't receive another request until after replying to the current one.

_requests_sent will have to become a map<uint32_t,
ref_ptr<RequestState>> to cope with out-of-order responses.  I will try
to fix that next.

Cheers,

Steven

--