[Xorp-hackers] Crash due to stale cached xrl sender pointer.

[Bruce Simpson]

Ben Greear wrote:
>
> With regard to larger issues of the IPC, I dislike the current callback
> logic.  I would like to get rid of all the auto-generated template code
> and use real callback objects.  Each xrl call will have a pointer to a 
> callback
> object and will call that at appropriate times.
>
> If/when you get your Thrift stuff in, I might attempt to work on
> the callback logic.

Can you expand on what a 'real callback object' is?
The callback() method in XORP is a template method which returns a real 
C++ object.

FWIW, Boost.Function is actually implemented in a pretty similar way to 
XORP's callback library:
    http://www.boost.org/doc/libs/1_40_0/doc/html/function.html

It is actually more generic than XORP's, which assumes that a callback() 
is never going to be invoked right away (like a functor).

The code already maintains a callback pointer for XRL invocation, see 
XrlSender::send(). Because Xrl is an intermediate representation, and is 
sometimes used synchronously, the callback isn't maintained there.

>
> I don't think we should worry about any async message support in the 
> transport
> level.  As long as we have useful callbacks, then only the 
> client/servers need
> to worry about async.  For instance, client says 'do-big-work()'.
> Server immediately responds 'working' (RPC call is now done).
> Later, server can send more messages to client as the big work completes.
>
> XRL doesn't need to know anything about this.

We already do. Asynchrony is just a fact of life in libxipc.

To dispel any confusion: I'm not referring to asynchrony at the level of 
a socket or a UNIX file descriptor. When discussing asynchrony in my 
posts, I am writing about mechanisms already present in the code.

It's been clear from past history, and private exchanges with other 
developers, that scalability has been a concern, particularly with BGP. 
Scalability is something I consider on the worktop now, and to be 
considered, but not necessarily taking action on it right now.

I am documenting what I'm seeing, as I'm reading further into it; the 
way we do XRL, does influence our scalability. We may not always use 
streams for the transport, for such reasons.

The big problem with Thrift has been that there is no asynchrony story 
in Thrift as it's currently shipped. As such, I'm 'retro-fitting' the 
protocol into XORP, along a principle of least invasive change.

I don't plan to change callback behaviour, nor other aspects of the 
existing programming model.

>
> If/when it's actually needed, we can do that work then.  (For threads,
> likely this would require a re-write of the entire application in
> question, as well as hacking on the various libraries.)

My current opinion is that making the tree thread-safe is feasible, but 
most likely means getting rid of some of the non-reentrant code which 
exists in libxorp.

Thread safety is very unlikely to involve a ground-up rewrite of the 
whole tree, if appropriate constraints are in place.

Boost could be leveraged to deliver some of that thread safety, but 
that's out-of-scope for the current effort.

>
> For instance, in my case, just adding that netlink filtering took 
> system load
> from 300+ to 20, effectively making an O(N^2) to O(N).  This sort of
> thing is way more important than saving a few cycles on encode/decode
> of messages we send once per second.

In the case of XRL, pack/unpack is probably not the bottleneck; 
allocations probably are.

I appreciate what you're doing with Netlink; I often wish myself that 
the BSD developers would just implement that API, as it would solve a 
number of problems they're now having with API stability.

>
> Most of the slowdowns I've seen are stupid things like sleeping for
> a timeout instead of doing work, probably to work around some race
> no one felt like debugging.

Were these timeouts actually blocking execution of other pending tasks?
If so, that's something which needs dealing with.

thanks,
BMS