Bruce Simpson wrote:
I'm running out the door and don't really have time to finish this patch.
But this is along the lines of what I'm really looking for.
That obviously didn't compile. There are some things to be aware of --
whilst you can test the value of a Boost smart pointer, and use it where
a pointer would be used, direct C-style comparisons sometimes need
explicit casting, or just use foo.get().
shared_ptr is used here only to manage the lifecycle of XrlPFSender
within XrlRouter, which owns the object. class Xrl had no business
holding an unchecked pointer to a shared object as a cache. weak_ptr
allows class Xrl's cached reference to be validated upfront, and makes
the intent behind it clearer in the source.
Attached is a patch which compiles, and passes a 'scons check'. If you
guys could test further, that would be great.
Points about using shared_ptr<T>/weak_ptr<T> vs ref_ptr<T>:
* Implementation is Boost community supported and mature.
* Implementation follows pimp/rimpl.
* Semantics are more explicit.
* weak_ptr observes a resource.
* shared_ptr owns a resource.
* Will use pthread locks to acquire resource if threaded;
if not, atomic machine-specific instructions will be used;
finally, portable spin-locks are used.
For now, Boost thread support is explicitly disabled.
However, the counter pool used by shared_ptr<T> is private to its
instantiation. A shared base class is used, but unlike ref_ptr<T>, we
don't explicitly instantiate it within shared code, so this leads to a
little template instantiation in libxorp_ipc.so for the time being.
I measure the binary size gain in libxorp_ipc.so on FreeBSD 8.0/amd64 (I
have now updated):
+8270 bytes text
+296 bytes data
I can live with that, as the race is fairly deep in the core.
No performance profiling of this change. A basic router appears to run
fine with this change.
General coding advice:
* Hold shared_ptr<T> across operations which require it.
* Use weak_ptr<T> in situations where the pointer is being cached or
crossing subsystem boundaries, or where it goes to existing,
non-threaded XORP APIs, e.g. callbacks.
* No need for enable_shared_from_this<T> yet, we aren't going to
push the weak_ptr further down.
Introduce the use of Boost's weak_ptr<T>/shared_ptr<T> for managing
the lifecycle of XrlPFSender objects.
class Xrl has been taught to hold a weak_ptr<XrlPFSender> for its cached,
resolved sender object.
The method in XrlRouter which tries to find an existing RPC endpoint for
outgoing XRLs has been renamed lookup_sender() just now, to avoid clashing
with the hooks for batch XRL support (which are not currently used). This
is probably a misnomer but we can come back to this later.
As most of the XORP code base is not yet thread or Boost aware, we
continue to use XrlPFSender* in most situations.
This fix is cumulative with the auto_ptr<> fix used in the XIF client-side
RPC stubs themselves, this is necessary to ensure the weak_ptr is private
to each client instantiation.
Reported by: Ben Greear, Li Zhao
Index: libxipc/xrl_router.hh
===================================================================
--- libxipc/xrl_router.hh (revision 11686)
+++ libxipc/xrl_router.hh (working copy)
@@ -34,7 +34,10 @@
#include "finder_constants.hh"
#include "finder_client_observer.hh"
+#include <boost/shared_ptr.hpp>
+using boost::shared_ptr;
+
class DispatchState;
class FinderClient;
@@ -190,6 +193,7 @@
/**
* Send callback (fast path).
+ * FIXME: use smart ptr.
*/
void send_callback(const XrlError& e,
XrlArgs* reply,
@@ -211,8 +215,9 @@
uint16_t finder_port);
private:
+ // XXX
XrlPFSender& get_sender(const string& target);
- XrlPFSender* get_sender(const Xrl& xrl, FinderDBEntry *dbe);
+ shared_ptr<XrlPFSender> lookup_sender(const Xrl& xrl, FinderDBEntry *dbe);
protected:
EventLoop& _e;
@@ -224,10 +229,12 @@
list<XrlPFListener*> _listeners; // listeners
list<XrlRouterDispatchState*> _dsl; // dispatch
state
- list<XrlPFSender*> _senders; // active senders
+ list< shared_ptr<XrlPFSender> > _senders; // active senders
static uint32_t _icnt; // instance count
+ // XXX the following are mostly only used by the incomplete
+ // batch support. Stick to using unchecked pointers for now --bms
private:
typedef map<string, XI*> XIM;
typedef map<string, XrlPFSender*> SENDERS;
Index: libxipc/xrl_pf_stcp.cc
===================================================================
--- libxipc/xrl_pf_stcp.cc (revision 11686)
+++ libxipc/xrl_pf_stcp.cc (working copy)
@@ -1083,12 +1083,17 @@
void
XrlPFSTCPSender::batch_start()
{
+#if 0
_batching = true;
+#else
+ XLOG_UNREACHABLE();
+#endif
}
void
XrlPFSTCPSender::batch_stop()
{
+#if 0
_batching = false;
// If we aint got no requests, we may not be able to signal to the receiver
@@ -1100,4 +1105,7 @@
if (_writer->running() == false)
_writer->start();
+#else
+ XLOG_UNREACHABLE();
+#endif
}
Index: libxipc/xrl_pf_factory.hh
===================================================================
--- libxipc/xrl_pf_factory.hh (revision 11686)
+++ libxipc/xrl_pf_factory.hh (working copy)
@@ -27,19 +27,20 @@
#include "xrl_error.hh"
#include "xrl_pf.hh"
+#include <boost/shared_ptr.hpp>
+
+using boost::shared_ptr;
+
class XrlPFSenderFactory {
public:
static void startup();
static void shutdown();
- static XrlPFSender* create_sender(EventLoop& eventloop,
- const char* proto_colon_addr);
+ static shared_ptr<XrlPFSender> create_sender(
+ EventLoop& eventloop, const char* proto_colon_addr);
- static XrlPFSender* create_sender(EventLoop& e,
- const char* protocol,
- const char* address);
-
- static void destroy_sender(XrlPFSender* s);
+ static shared_ptr<XrlPFSender> create_sender(EventLoop& e,
+ const char* protocol, const char* address);
};
#endif // __LIBXIPC_XRL_PF_FACTORY_HH__
Index: libxipc/xrl.cc
===================================================================
--- libxipc/xrl.cc (revision 11686)
+++ libxipc/xrl.cc (working copy)
@@ -76,7 +76,7 @@
const XrlArgs& args)
: _protocol(protocol), _target(protocol_target), _command(command),
_args(args), _sna_atom(NULL), _packed_bytes(0), _argp(&_args),
- _to_finder(-1), _resolved(false), _resolved_sender(NULL)
+ _to_finder(-1), _resolved(false)
{
}
@@ -85,7 +85,7 @@
const XrlArgs& args)
: _protocol(_finder_protocol), _target(target), _command(command),
_args(args), _sna_atom(NULL), _packed_bytes(0), _argp(&_args),
- _to_finder(-1), _resolved(false), _resolved_sender(NULL)
+ _to_finder(-1), _resolved(false)
{
}
@@ -94,7 +94,7 @@
const string& command)
: _protocol(protocol), _target(protocol_target), _command(command),
_sna_atom(NULL), _packed_bytes(0), _argp(&_args), _to_finder(-1),
- _resolved(false), _resolved_sender(NULL)
+ _resolved(false)
{
}
@@ -102,7 +102,7 @@
const string& command)
: _protocol(_finder_protocol), _target(target), _command(command),
_sna_atom(NULL), _packed_bytes(0), _argp(&_args), _to_finder(-1),
- _resolved(false), _resolved_sender(NULL)
+ _resolved(false)
{
}
@@ -110,13 +110,13 @@
const char* command)
: _protocol(_finder_protocol), _target(target), _command(command),
_sna_atom(NULL), _packed_bytes(0), _argp(&_args), _to_finder(-1),
- _resolved(false), _resolved_sender(NULL)
+ _resolved(false)
{
}
Xrl::Xrl(const char* c_str) throw (InvalidString)
: _sna_atom(NULL), _packed_bytes(0), _argp(&_args),
- _to_finder(-1), _resolved(false), _resolved_sender(NULL)
+ _to_finder(-1), _resolved(false)
{
if (0 == c_str)
xorp_throw0(InvalidString);
@@ -136,7 +136,7 @@
Xrl::Xrl()
: _sna_atom(0), _packed_bytes(0), _argp(&_args), _to_finder(-1),
- _resolved(false), _resolved_sender(NULL)
+ _resolved(false)
{
}
@@ -335,8 +335,9 @@
_packed_bytes = 0;
_to_finder = -1;
_resolved = false;
- _resolved_sender = NULL;
+ _resolved_sender.reset();
+
delete _sna_atom;
_sna_atom = NULL;
}
Index: libxipc/xrl_pf.cc
===================================================================
--- libxipc/xrl_pf.cc (revision 11686)
+++ libxipc/xrl_pf.cc (working copy)
@@ -55,4 +55,5 @@
XrlPFSender::~XrlPFSender()
{
+ // XXX put a debug_msg() here; we are now deleted through shared_ptr.
}
Index: libxipc/xrl.hh
===================================================================
--- libxipc/xrl.hh (revision 11686)
+++ libxipc/xrl.hh (working copy)
@@ -26,11 +26,15 @@
#include <string>
+#include <boost/weak_ptr.hpp>
+
#include "libxorp/exceptions.hh"
#include "xrl_atom.hh"
#include "xrl_args.hh"
#include "xrl_tokens.hh"
+using boost::weak_ptr;
+
class XrlPFSender;
/**
@@ -169,9 +173,14 @@
bool resolved() const { return _resolved; }
void set_resolved(bool r) const { _resolved = r; }
- XrlPFSender *resolved_sender() const { return _resolved_sender; }
- void set_resolved_sender(XrlPFSender *s) const { _resolved_sender = s; }
+ weak_ptr<XrlPFSender> resolved_sender() const {
+ return _resolved_sender;
+ }
+ void set_resolved_sender(weak_ptr<XrlPFSender> s) const {
+ _resolved_sender = s;
+ }
+
void set_target(const char* target);
private:
@@ -194,7 +203,7 @@
mutable XrlArgs* _argp; // XXX shouldn't be mutable
mutable int _to_finder;
mutable bool _resolved; // XXX ditto
- mutable XrlPFSender* _resolved_sender; // XXX ditto
+ mutable weak_ptr<XrlPFSender> _resolved_sender; // XXX ditto
static const string _finder_protocol;
};
Index: libxipc/xrl_pf.hh
===================================================================
--- libxipc/xrl_pf.hh (revision 11686)
+++ libxipc/xrl_pf.hh (working copy)
@@ -105,7 +105,15 @@
virtual bool sends_pending() const = 0;
virtual const char* protocol() const = 0;
+
+ /**
+ * Determine if the underlying transport is still open.
+ *
+ * @return true if the transport is alive.
+ */
virtual bool alive() const = 0;
+
+ // XXX Unfinished support for XRL batching.
virtual void batch_start() {}
virtual void batch_stop() {}
Index: libxipc/xrl_router.cc
===================================================================
--- libxipc/xrl_router.cc (revision 11686)
+++ libxipc/xrl_router.cc (working copy)
@@ -38,6 +38,10 @@
#include "sockutil.hh"
+// XXX should be included nested above.
+//#include <boost/shared_ptr.hpp>
+//#include <boost/weak_ptr.hpp>
+
//
// Enable this macro to enable Xrl callback checker that checks each Xrl
// callback is dispatched just once.
@@ -273,10 +277,8 @@
_fc->detach_observer(this);
_fac->set_enabled(false);
- while (_senders.empty() == false) {
- XrlPFSenderFactory::destroy_sender(_senders.front());
+ while (_senders.empty() == false)
_senders.pop_front();
- }
while (_dsl.empty() == false) {
delete _dsl.front();
@@ -389,7 +391,8 @@
bool direct_call)
{
try {
- XrlPFSender* s = get_sender(xrl, const_cast<FinderDBEntry*>(dbe));
+ shared_ptr<XrlPFSender> s =
+ lookup_sender(xrl, const_cast<FinderDBEntry*>(dbe));
if (s == 0) {
// Notify Finder client that result was bad.
_fc->uncache_result(dbe);
@@ -400,10 +403,11 @@
const Xrl& x = dbe->xrls().front();
x.set_args(xrl);
- if (s) {
+ if (s != 0) {
trace_xrl("Sending ", x);
return s->send(x, direct_call,
- callback(this, &XrlRouter::send_callback, s, cb));
+ callback(this, &XrlRouter::send_callback,
+ s.get(), cb));
}
cb->dispatch(XrlError(SEND_FAILED, "sender not instantiated"), 0);
} catch (const InvalidString&) {
@@ -412,31 +416,47 @@
return false;
}
-XrlPFSender*
-XrlRouter::get_sender(const Xrl& xrl, FinderDBEntry* dbe)
+// XXX Return uninitialized shared_ptr, or locked shared_ptr,
+// on desired XrlPFSender.
+shared_ptr<XrlPFSender>
+XrlRouter::lookup_sender(const Xrl& xrl, FinderDBEntry* dbe)
{
const Xrl& x = dbe->xrls().front();
- XrlPFSender* s = NULL;
+ shared_ptr<XrlPFSender> s;
- // Use the cache pointer to the sender.
+ // Try to use the cached pointer to the sender.
if (xrl.resolved()) {
- s = xrl.resolved_sender();
+ weak_ptr<XrlPFSender> w = xrl.resolved_sender();
- if (s->alive())
+ // Check if cached weak_ptr to sender is still valid; we may
+ // have lost a race on the XRLDB.
+ // Check if transport layer is still up for this sender.
+ // If good to go, return shared pointer for which we
+ // still hold the lock.
+ s = w.lock();
+ if (s.get() != 0 && s->alive())
return s;
+ // We lost the race.
+
+ // XXX Sanity check that this sender's protocol and endpoint
+ // addresses are the same as they were when the Xrl was
+ // instantiated.
XLOG_ASSERT(s->protocol() == x.protocol());
XLOG_ASSERT(s->address() == x.target());
xrl.set_resolved(false);
- xrl.set_resolved_sender(NULL);
+ xrl.set_resolved_sender(weak_ptr<XrlPFSender>());
}
// Find a new sender.
- for (list<XrlPFSender*>::iterator i = _senders.begin();
+ for (list< shared_ptr<XrlPFSender> >::iterator i = _senders.begin();
i != _senders.end(); ++i) {
s = *i;
+ // This XrlRouter holds the reference, so the pointer should be valid.
+ XLOG_ASSERT(s.get() != 0);
+
if (s->protocol() != x.protocol() || s->address() != x.target())
continue;
@@ -448,13 +468,13 @@
XLOG_INFO("Sender died (protocol = \"%s\", address = \"%s\")",
s->protocol(), s->address().c_str());
- XrlPFSenderFactory::destroy_sender(s);
+
_senders.erase(i);
_senders2.erase(xrl.target());
break;
}
- s = NULL;
+ s.reset();
// create sender
while (dbe->xrls().size()) {
@@ -462,7 +482,7 @@
s = XrlPFSenderFactory::create_sender(_e, x.protocol().c_str(),
x.target().c_str());
- if (s)
+ if (s.get() != 0)
break;
XLOG_ERROR("Could not create XrlPFSender for protocol = \"%s\" "
@@ -471,15 +491,18 @@
dbe->pop_front();
}
- if (!s)
- return NULL;
+ // Unable to instantiate.
+ if (s == 0)
+ return shared_ptr<XrlPFSender>();
+
+ // New sender instantiated, take lock and record state.
const Xrl& front = dbe->xrls().front();
XLOG_ASSERT(s->protocol() == front.protocol());
XLOG_ASSERT(s->address() == front.target());
_senders.push_back(s);
- _senders2[xrl.target()] = s;
+ _senders2[xrl.target()] = s.get();
// Don't do this here as it is set in the Xrl that is stored with
// the finder client, so is not used. But if the connection needs
@@ -506,7 +529,7 @@
if (e == XrlError::OKAY()) {
const Xrl& xrl = ds->xrl();
xrl.set_resolved(false);
- xrl.set_resolved_sender(NULL);
+ xrl.set_resolved_sender(weak_ptr<XrlPFSender>());
if (send_resolved(xrl, dbe, ds->cb(), false) == false) {
// We tried to force sender to send xrl and it declined the
// opportunity. This should only happen when it's out of buffer
@@ -710,25 +733,44 @@
debug_msg("Finder target ready event: \"%s\"\n", tgt_name.c_str());
}
+//
+// XXX This is unfinished code related to a batch XRL implementation.
+// It is unfortunately incompatible, straight off, with using refcounts to
+// manage XrlPFSender's lifecycle, so it needs to be revisited later by
+// an interested party.
+// Also, the overloading of the get_sender() method name is potentially
+// confusing. --bms
+
void
XrlRouter::batch_start(const string& target)
{
+#if 0
XrlPFSender& sender = get_sender(target);
sender.batch_start();
+#else
+ XLOG_UNREACHABLE();
+ UNUSED(target);
+#endif
}
void
XrlRouter::batch_stop(const string& target)
{
+#if 0
XrlPFSender& sender = get_sender(target);
sender.batch_stop();
+#else
+ XLOG_UNREACHABLE();
+ UNUSED(target);
+#endif
}
XrlPFSender&
XrlRouter::get_sender(const string& target)
{
+#if 0
XrlPFSender* s = 0;
SENDERS::iterator i = _senders2.find(target);
@@ -737,6 +779,12 @@
s = i->second;
return *s;
+#else
+ XLOG_UNREACHABLE();
+ XrlPFSender* s = 0;
+ return *s;
+ UNUSED(target);
+#endif
}
Index: libxipc/xrl_pf_factory.cc
===================================================================
--- libxipc/xrl_pf_factory.cc (revision 11686)
+++ libxipc/xrl_pf_factory.cc (working copy)
@@ -33,12 +33,14 @@
#include "xrl_pf_stcp.hh"
#include "xrl_pf_unix.hh"
+//#include <boost/make_shared.hpp>
+//using boost::make_shared;
// STCP senders are a special case. Constructing an STCP sender has
// real cost, unlike InProc and SUDP, so we maintain a cache of
// STCP senders with one per sender destination address.
-XrlPFSender*
+shared_ptr<XrlPFSender>
XrlPFSenderFactory::create_sender(EventLoop& eventloop,
const char* protocol,
const char* address)
@@ -47,18 +49,22 @@
protocol, address);
try {
if (strcmp(XrlPFSTCPSender::protocol_name(), protocol) == 0) {
- return new XrlPFSTCPSender(eventloop, address);
+ // XXX boost::make_shared() candidate.
+ return shared_ptr<XrlPFSender>(
+ new XrlPFSTCPSender(eventloop, address));
}
if (strcmp(XrlPFUNIXSender::protocol_name(), protocol) == 0) {
- return new XrlPFUNIXSender(eventloop, address);
+ // XXX boost::make_shared() candidate.
+ return shared_ptr<XrlPFSender>(
+ new XrlPFUNIXSender(eventloop, address));
}
} catch (XorpException& e) {
XLOG_ERROR("XrlPFSenderFactory::create failed: %s\n", e.str().c_str());
}
- return 0;
+ return shared_ptr<XrlPFSender>();
}
-XrlPFSender*
+shared_ptr<XrlPFSender>
XrlPFSenderFactory::create_sender(EventLoop& eventloop,
const char* protocol_colon_address)
{
@@ -66,7 +72,7 @@
if (colon == 0) {
debug_msg("No colon in supposedly colon separated <protocol><address>"
"combination\n\t\"%s\".\n", protocol_colon_address);
- return 0;
+ return shared_ptr<XrlPFSender>();
}
string protocol(protocol_colon_address, colon - protocol_colon_address);
@@ -74,15 +80,6 @@
}
void
-XrlPFSenderFactory::destroy_sender(XrlPFSender* s)
-{
- debug_msg("destroying sender pf = \"%s\", addr = \"%s\"\n",
- s->protocol(), s->address().c_str());
-
- delete s;
-}
-
-void
XrlPFSenderFactory::startup()
{
}
_______________________________________________
Xorp-hackers mailing list
[email protected]
http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers
