Ben Greear wrote:
XRL caches a pointer to the resolved_sender, but when something
deletes a sender, it doesn't appear to clean up any existing XRLs.
This leads to a crash on a highly loaded system (where senders must be
timing out
or something like that).
Looks like a good place for smart pointers. I'm going to attempt that
unless
someone has another idea...
The attached patch seems to fix the problem.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
diff --git a/libxipc/xrl.cc b/libxipc/xrl.cc
index 187c7b1..5de3e1d 100644
--- a/libxipc/xrl.cc
+++ b/libxipc/xrl.cc
@@ -76,7 +76,7 @@ Xrl::Xrl(const string& protocol,
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 @@ Xrl::Xrl(const string& target,
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 @@ Xrl::Xrl(const string& protocol,
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 @@ Xrl::Xrl(const string& target,
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 @@ Xrl::Xrl(const char* target,
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(const char* c_str) throw (InvalidString)
Xrl::Xrl()
: _sna_atom(0), _packed_bytes(0), _argp(&_args), _to_finder(-1),
- _resolved(false), _resolved_sender(NULL)
+ _resolved(false)
{
}
@@ -340,3 +340,15 @@ Xrl::clear_cache()
delete _sna_atom;
_sna_atom = NULL;
}
+
+ref_ptr<XrlPFSender> Xrl::resolved_sender_ref() const {
+ return _resolved_sender;
+}
+
+void Xrl::set_resolved_sender_ref(ref_ptr<XrlPFSender>& s) const {
+ _resolved_sender = s;
+}
+
+void Xrl::set_resolved_sender_ptr(XrlPFSender* s) const {
+ _resolved_sender = s;
+}
diff --git a/libxipc/xrl.hh b/libxipc/xrl.hh
index bb8052e..2eec2b6 100644
--- a/libxipc/xrl.hh
+++ b/libxipc/xrl.hh
@@ -30,8 +30,8 @@
#include "xrl_atom.hh"
#include "xrl_args.hh"
#include "xrl_tokens.hh"
-
-class XrlPFSender;
+#include "libxorp/ref_ptr.hh"
+#include "xrl_pf.hh" // Needed for ref_ptr instantiation of XrlPFSender
/**
* XORP IPC request.
@@ -169,8 +169,9 @@ public:
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; }
+ ref_ptr<XrlPFSender> resolved_sender_ref() const;
+ void set_resolved_sender_ref(ref_ptr<XrlPFSender>& s) const;
+ void set_resolved_sender_ptr(XrlPFSender* s) const;
void set_target(const char* target);
@@ -194,7 +195,7 @@ private:
mutable XrlArgs* _argp; // XXX shouldn't be mutable
mutable int _to_finder;
mutable bool _resolved; // XXX ditto
- mutable XrlPFSender* _resolved_sender; // XXX ditto
+ mutable ref_ptr<XrlPFSender> _resolved_sender; // XXX ditto
static const string _finder_protocol;
};
diff --git a/libxipc/xrl_pf.hh b/libxipc/xrl_pf.hh
index b185ee5..ae34e36 100644
--- a/libxipc/xrl_pf.hh
+++ b/libxipc/xrl_pf.hh
@@ -108,6 +108,7 @@ public:
const string& address() const { return _address; }
EventLoop& eventloop() const { return _eventloop; }
+ virtual void set_address(const char* a) { _address = a; }
private:
XrlPFSender(const XrlPFSender&); // Not implemented
diff --git a/libxipc/xrl_pf_factory.cc b/libxipc/xrl_pf_factory.cc
index 01cb230..9cd57f4 100644
--- a/libxipc/xrl_pf_factory.cc
+++ b/libxipc/xrl_pf_factory.cc
@@ -41,13 +41,14 @@
// real cost, unlike InProc and SUDP, so we maintain a cache of
// STCP senders with one per sender destination address.
-XrlPFSender*
+ref_ptr<XrlPFSender>
XrlPFSenderFactory::create_sender(EventLoop& eventloop,
const char* protocol,
const char* address)
{
debug_msg("instantiating sender pf = \"%s\", addr = \"%s\"\n",
protocol, address);
+ ref_ptr<XrlPFSender> rv;
try {
#if 0
if (strcmp(XrlPFSUDPSender::protocol_name(), protocol) == 0) {
@@ -55,7 +56,8 @@ XrlPFSenderFactory::create_sender(EventLoop& eventloop,
} else
#endif
if (strcmp(XrlPFSTCPSender::protocol_name(), protocol) == 0) {
- return new XrlPFSTCPSender(eventloop, address);
+ rv = new XrlPFSTCPSender(eventloop, address);
+ return rv;
}
#if 0
else if (strcmp(XrlPFInProcSender::protocol_name(), protocol) == 0) {
@@ -64,15 +66,17 @@ XrlPFSenderFactory::create_sender(EventLoop& eventloop,
return new XrlPFKillSender(eventloop, address);
} else
#endif
- if (strcmp(XrlPFUNIXSender::protocol_name(), protocol) == 0)
- return new XrlPFUNIXSender(eventloop, address);
+ if (strcmp(XrlPFUNIXSender::protocol_name(), protocol) == 0) {
+ rv = new XrlPFUNIXSender(eventloop, address);
+ return rv;
+ }
} catch (XorpException& e) {
XLOG_ERROR("XrlPFSenderFactory::create failed: %s\n", e.str().c_str());
}
- return 0;
+ return rv;
}
-XrlPFSender*
+ref_ptr<XrlPFSender>
XrlPFSenderFactory::create_sender(EventLoop& eventloop,
const char* protocol_colon_address)
{
@@ -80,7 +84,8 @@ XrlPFSenderFactory::create_sender(EventLoop& eventloop,
if (colon == 0) {
debug_msg("No colon in supposedly colon separated <protocol><address>"
"combination\n\t\"%s\".\n", protocol_colon_address);
- return 0;
+ ref_ptr<XrlPFSender> rv;
+ return rv;
}
string protocol(protocol_colon_address, colon - protocol_colon_address);
@@ -88,12 +93,12 @@ XrlPFSenderFactory::create_sender(EventLoop& eventloop,
}
void
-XrlPFSenderFactory::destroy_sender(XrlPFSender* s)
+XrlPFSenderFactory::destroy_sender(ref_ptr<XrlPFSender>& s)
{
debug_msg("destroying sender pf = \"%s\", addr = \"%s\"\n",
s->protocol(), s->address().c_str());
-
- delete s;
+ s->set_address("__DESTROYED__"); // for debugging.
+ // Ref-ptrs should clean themselves up..don't really need to do anything here.
}
void
diff --git a/libxipc/xrl_pf_factory.hh b/libxipc/xrl_pf_factory.hh
index 1b877db..41b737d 100644
--- a/libxipc/xrl_pf_factory.hh
+++ b/libxipc/xrl_pf_factory.hh
@@ -32,14 +32,14 @@ public:
static void startup();
static void shutdown();
- static XrlPFSender* create_sender(EventLoop& eventloop,
+ static ref_ptr<XrlPFSender> create_sender(EventLoop& eventloop,
const char* proto_colon_addr);
- static XrlPFSender* create_sender(EventLoop& e,
+ static ref_ptr<XrlPFSender> create_sender(EventLoop& e,
const char* protocol,
const char* address);
- static void destroy_sender(XrlPFSender* s);
+ static void destroy_sender(ref_ptr<XrlPFSender>& s);
};
#endif // __LIBXIPC_XRL_PF_FACTORY_HH__
diff --git a/libxipc/xrl_router.cc b/libxipc/xrl_router.cc
index 500c2ad..5f621c6 100644
--- a/libxipc/xrl_router.cc
+++ b/libxipc/xrl_router.cc
@@ -375,7 +375,7 @@ XrlRouter::add_handler(const string& cmd, const XrlRecvCallback& rcb)
void
XrlRouter::send_callback(const XrlError& e,
XrlArgs* reply,
- XrlPFSender* /* s */,
+ XrlPFSender* /* s */, // Don't use, should be a ref-ptr if it is ever used.
XrlCallback user_callback)
{
user_callback->dispatch(e, reply);
@@ -388,8 +388,8 @@ XrlRouter::send_resolved(const Xrl& xrl,
bool direct_call)
{
try {
- XrlPFSender* s = get_sender(xrl, const_cast<FinderDBEntry*>(dbe));
- if (s == 0) {
+ ref_ptr<XrlPFSender> s = get_sender(xrl, const_cast<FinderDBEntry*>(dbe));
+ if (!s.get()) {
// Notify Finder client that result was bad.
_fc->uncache_result(dbe);
@@ -399,10 +399,13 @@ XrlRouter::send_resolved(const Xrl& xrl,
const Xrl& x = dbe->xrls().front();
x.set_args(xrl);
- if (s) {
+ if (s.get()) {
trace_xrl("Sending ", x);
+ // NOTE: using s.get below breaks ref counting, but can't figure out WTF the
+ // callback template magic is to make it work with a ref-ptr. Either way, it appears
+ // the ptr may not be used anyway (it's not in send_callback, for instance)
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&) {
@@ -411,28 +414,34 @@ XrlRouter::send_resolved(const Xrl& xrl,
return false;
}
-XrlPFSender*
+ref_ptr<XrlPFSender>
XrlRouter::get_sender(const Xrl& xrl, FinderDBEntry* dbe)
{
const Xrl& x = dbe->xrls().front();
- XrlPFSender* s = NULL;
+ ref_ptr<XrlPFSender> s;
// Use the cache pointer to the sender.
if (xrl.resolved()) {
- s = xrl.resolved_sender();
+ s = xrl.resolved_sender_ref();
if (s->alive())
return s;
- XLOG_ASSERT(s->protocol() == x.protocol());
- XLOG_ASSERT(s->address() == x.target());
+ if (strcmp(s->protocol(), x.protocol().c_str()) != 0) {
+ XLOG_WARNING("ERROR: x->protocol: %s doesn't match x.protocol: %s\n",
+ s->protocol(), x.protocol().c_str());
+ }
+ if (s->address() != x.target()) {
+ XLOG_WARNING("ERROR: s->address: %s doesn't match x.target: %s\n",
+ s->address().c_str(), x.target().c_str());
+ }
xrl.set_resolved(false);
- xrl.set_resolved_sender(NULL);
+ xrl.set_resolved_sender_ptr(NULL);
}
// Find a new sender.
- for (list<XrlPFSender*>::iterator i = _senders.begin();
+ for (list<ref_ptr<XrlPFSender> >::iterator i = _senders.begin();
i != _senders.end(); ++i) {
s = *i;
@@ -441,7 +450,7 @@ XrlRouter::get_sender(const Xrl& xrl, FinderDBEntry* dbe)
if (s->alive()) {
xrl.set_resolved(true);
- xrl.set_resolved_sender(s);
+ xrl.set_resolved_sender_ref(s);
return s;
}
@@ -461,7 +470,7 @@ XrlRouter::get_sender(const Xrl& xrl, FinderDBEntry* dbe)
s = XrlPFSenderFactory::create_sender(_e, x.protocol().c_str(),
x.target().c_str());
- if (s)
+ if (s.get())
break;
XLOG_ERROR("Could not create XrlPFSender for protocol = \"%s\" "
@@ -470,8 +479,8 @@ XrlRouter::get_sender(const Xrl& xrl, FinderDBEntry* dbe)
dbe->pop_front();
}
- if (!s)
- return NULL;
+ if (!s.get())
+ return s;
const Xrl& front = dbe->xrls().front();
@@ -505,7 +514,7 @@ XrlRouter::resolve_callback(const XrlError& e,
if (e == XrlError::OKAY()) {
const Xrl& xrl = ds->xrl();
xrl.set_resolved(false);
- xrl.set_resolved_sender(NULL);
+ xrl.set_resolved_sender_ptr(NULL);
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
@@ -712,30 +721,32 @@ XrlRouter::finder_ready_event(const string& tgt_name)
void
XrlRouter::batch_start(const string& target)
{
- XrlPFSender& sender = get_sender(target);
-
- sender.batch_start();
+ ref_ptr<XrlPFSender> sender = get_sender(target);
+ if (sender.get()) {
+ sender->batch_start();
+ }
}
void
XrlRouter::batch_stop(const string& target)
{
- XrlPFSender& sender = get_sender(target);
-
- sender.batch_stop();
+ ref_ptr<XrlPFSender> sender = get_sender(target);
+ if (sender.get()) {
+ sender->batch_stop();
+ }
}
-XrlPFSender&
+ref_ptr<XrlPFSender>
XrlRouter::get_sender(const string& target)
{
- XrlPFSender* s = 0;
+ ref_ptr<XrlPFSender> s;
SENDERS::iterator i = _senders2.find(target);
XLOG_ASSERT(i != _senders2.end());
s = i->second;
- return *s;
+ return s;
}
string XrlRouter::toString() const {
diff --git a/libxipc/xrl_router.hh b/libxipc/xrl_router.hh
index 29ef12a..7af43b6 100644
--- a/libxipc/xrl_router.hh
+++ b/libxipc/xrl_router.hh
@@ -195,7 +195,7 @@ protected:
*/
void send_callback(const XrlError& e,
XrlArgs* reply,
- XrlPFSender* sender,
+ XrlPFSender* sender, // un-used, should be ref-ptr if we ever actually use this.
XrlCallback user_callback);
/**
@@ -213,8 +213,8 @@ protected:
uint16_t finder_port);
private:
- XrlPFSender& get_sender(const string& target);
- XrlPFSender* get_sender(const Xrl& xrl, FinderDBEntry *dbe);
+ ref_ptr<XrlPFSender> get_sender(const string& target);
+ ref_ptr<XrlPFSender> get_sender(const Xrl& xrl, FinderDBEntry *dbe);
protected:
EventLoop& _e;
@@ -226,13 +226,13 @@ protected:
list<XrlPFListener*> _listeners; // listeners
list<XrlRouterDispatchState*> _dsl; // dispatch state
- list<XrlPFSender*> _senders; // active senders
+ list<ref_ptr<XrlPFSender> > _senders; // active senders
static uint32_t _icnt; // instance count
private:
typedef map<string, XI*> XIM;
- typedef map<string, XrlPFSender*> SENDERS;
+ typedef map<string, ref_ptr<XrlPFSender> > SENDERS;
mutable XIM _xi_cache;
SENDERS _senders2;
_______________________________________________
Xorp-hackers mailing list
[email protected]
http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers
