From 07374496e73e8215390e79049cd3598c66639e1d Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Wed, 12 Oct 2016 10:28:15 -0700 Subject: [PATCH] Fix concurrency issues in caching string representation Caching the representation in a lazy manner when ToString/ToUVE etc. are called is prone to concurrency issues. Initialize the string representation in the constructors for the following: - String representation for BgpPeer ToString and ToUVEKey - String representation for InetRoute and Inet6Route ToString - String representation for XmppConnection ToUVEKey Change-Id: I01df9271d76a4c03977f1cc0618f97179f283999 Closes-Bug: 1632519 --- src/bgp/bgp_peer.cc | 38 ++++++++++++------------------------ src/bgp/bgp_peer.h | 4 ++-- src/bgp/inet/inet_route.cc | 10 +++------- src/bgp/inet/inet_route.h | 4 ++-- src/bgp/inet6/inet6_route.cc | 10 +++------- src/bgp/inet6/inet6_route.h | 4 ++-- src/xmpp/xmpp_connection.cc | 15 +++----------- src/xmpp/xmpp_connection.h | 1 - 8 files changed, 28 insertions(+), 58 deletions(-) diff --git a/src/bgp/bgp_peer.cc b/src/bgp/bgp_peer.cc index 68948942104..0fb09972172 100644 --- a/src/bgp/bgp_peer.cc +++ b/src/bgp/bgp_peer.cc @@ -596,6 +596,19 @@ BgpPeer::BgpPeer(BgpServer *server, RoutingInstance *instance, total_flap_count_(0), last_flap_(0), inuse_authkey_type_(AuthenticationData::NIL) { + ostringstream oss1; + oss1 << peer_key_.endpoint.address(); + if (peer_key_.endpoint.port() != BgpConfigManager::kDefaultPort) + oss1 << ":" << dec << peer_key_.endpoint.port(); + to_str_ = oss1.str(); + + ostringstream oss2; + if (rtinstance_) + oss2 << rtinstance_->name() << ":"; + oss2 << server_->localname() << ":"; + oss2 << peer_name(); + uve_key_str_ = oss2.str(); + membership_req_pending_ = 0; BGP_LOG_PEER(Event, this, SandeshLevel::SYS_INFO, BGP_LOG_FLAG_ALL, BGP_PEER_DIR_NA, "Created"); @@ -2129,31 +2142,6 @@ bool BgpPeer::ReceiveMsg(BgpSession *session, const u_int8_t *msg, return true; } -string BgpPeer::ToString() const { - if (to_str_.empty()) { - ostringstream out; - out << peer_key_.endpoint.address(); - if (peer_key_.endpoint.port() != BgpConfigManager::kDefaultPort) { - out << ":" << dec << peer_key_.endpoint.port(); - } - to_str_ = out.str(); - } - return to_str_; -} - -string BgpPeer::ToUVEKey() const { - if (uve_key_str_.empty()) { - ostringstream out; - if (rtinstance_) { - out << rtinstance_->name() << ":"; - } - out << server_->localname() << ":"; - out << peer_name(); - uve_key_str_ = out.str(); - } - return uve_key_str_; -} - // // Extract nexthop address from BgpMpNlri if appropriate and return updated // BgpAttrPtr. The original attribute is returned for cases where there's no diff --git a/src/bgp/bgp_peer.h b/src/bgp/bgp_peer.h index c4b5debc24b..e66670cb90e 100644 --- a/src/bgp/bgp_peer.h +++ b/src/bgp/bgp_peer.h @@ -88,8 +88,8 @@ class BgpPeer : public IPeer { // Interface methods // thread-safe - virtual std::string ToString() const; - virtual std::string ToUVEKey() const; + virtual std::string ToString() const { return to_str_; } + virtual std::string ToUVEKey() const { return uve_key_str_; } // thread: bgp::SendTask // Used to send an UPDATE message on the socket. diff --git a/src/bgp/inet/inet_route.cc b/src/bgp/inet/inet_route.cc index 17375f72de4..b73ea4af3b9 100644 --- a/src/bgp/inet/inet_route.cc +++ b/src/bgp/inet/inet_route.cc @@ -77,7 +77,9 @@ bool Ip4Prefix::IsMoreSpecific(const Ip4Prefix &rhs) const { (rhs.ip4_addr().to_ulong() & mask); } -InetRoute::InetRoute(const Ip4Prefix &prefix) : prefix_(prefix) { +InetRoute::InetRoute(const Ip4Prefix &prefix) + : prefix_(prefix), + prefix_str_(prefix.ToString()) { } int InetRoute::CompareTo(const Route &rhs) const { @@ -85,12 +87,6 @@ int InetRoute::CompareTo(const Route &rhs) const { return prefix_.CompareTo(rt_other.prefix_); } -string InetRoute::ToString() const { - if (prefix_str_.empty()) - prefix_str_ = prefix_.ToString(); - return prefix_str_; -} - // Check whether 'this' is more specific than rhs. bool InetRoute::IsMoreSpecific(const string &match) const { boost::system::error_code ec; diff --git a/src/bgp/inet/inet_route.h b/src/bgp/inet/inet_route.h index a48e350e4e8..b84d388267c 100644 --- a/src/bgp/inet/inet_route.h +++ b/src/bgp/inet/inet_route.h @@ -64,7 +64,7 @@ class InetRoute : public BgpRoute { public: explicit InetRoute(const Ip4Prefix &prefix); virtual int CompareTo(const Route &rhs) const; - virtual std::string ToString() const; + virtual std::string ToString() const { return prefix_str_; } const Ip4Prefix &GetPrefix() const { return prefix_; } @@ -90,7 +90,7 @@ class InetRoute : public BgpRoute { private: Ip4Prefix prefix_; - mutable std::string prefix_str_; + std::string prefix_str_; DISALLOW_COPY_AND_ASSIGN(InetRoute); }; diff --git a/src/bgp/inet6/inet6_route.cc b/src/bgp/inet6/inet6_route.cc index e55bdb64f43..ca1f93ffb96 100644 --- a/src/bgp/inet6/inet6_route.cc +++ b/src/bgp/inet6/inet6_route.cc @@ -91,7 +91,9 @@ Inet6Prefix Inet6Prefix::operator&(const Inet6Prefix& right) const { // Routines for class Inet6Route -Inet6Route::Inet6Route(const Inet6Prefix &prefix) : prefix_(prefix) { +Inet6Route::Inet6Route(const Inet6Prefix &prefix) + : prefix_(prefix), + prefix_str_(prefix.ToString()) { } int Inet6Route::CompareTo(const Route &rhs) const { @@ -99,12 +101,6 @@ int Inet6Route::CompareTo(const Route &rhs) const { return prefix_.CompareTo(rt_other.prefix_); } -string Inet6Route::ToString() const { - if (prefix_str_.empty()) - prefix_str_ = prefix_.ToString(); - return prefix_str_; -} - // Check whether 'this' is more specific than rhs. bool Inet6Route::IsMoreSpecific(const string &match) const { error_code ec; diff --git a/src/bgp/inet6/inet6_route.h b/src/bgp/inet6/inet6_route.h index a6a5d2b0bec..e5f5b5abd2e 100644 --- a/src/bgp/inet6/inet6_route.h +++ b/src/bgp/inet6/inet6_route.h @@ -73,7 +73,7 @@ class Inet6Route : public BgpRoute { public: explicit Inet6Route(const Inet6Prefix &prefix); virtual int CompareTo(const Route &rhs) const; - virtual std::string ToString() const; + virtual std::string ToString() const { return prefix_str_; } const Inet6Prefix &GetPrefix() const { return prefix_; } @@ -100,7 +100,7 @@ class Inet6Route : public BgpRoute { private: Inet6Prefix prefix_; - mutable std::string prefix_str_; + std::string prefix_str_; DISALLOW_COPY_AND_ASSIGN(Inet6Route); }; diff --git a/src/xmpp/xmpp_connection.cc b/src/xmpp/xmpp_connection.cc index 15b63786c01..cfd0518a371 100644 --- a/src/xmpp/xmpp_connection.cc +++ b/src/xmpp/xmpp_connection.cc @@ -58,6 +58,9 @@ XmppConnection::XmppConnection(TcpServer *server, state_machine_(XmppObjectFactory::Create( this, config->ClientOnly(), config->auth_enabled)), mux_(XmppObjectFactory::Create(this)) { + ostringstream oss; + oss << FromString() << ":" << endpoint().address().to_string(); + uve_key_str_ = oss.str(); } XmppConnection::~XmppConnection() { @@ -186,21 +189,9 @@ std::string XmppConnection::ToString() const { } std::string XmppConnection::ToUVEKey() const { - if (uve_key_str_.empty()) { - std::ostringstream out; - out << FromString() << ":" << endpoint().address().to_string(); - uve_key_str_ = out.str(); - } return uve_key_str_; } -void XmppConnection::SetFrom(const string &from) { - if (from_.size() == 0) { - from_ = from; - state_machine_->Initialize(); - } -} - static void XMPPPeerInfoSend(XmppPeerInfoData &peer_info) { XMPPPeerInfo::Send(peer_info); } diff --git a/src/xmpp/xmpp_connection.h b/src/xmpp/xmpp_connection.h index c60e34c468a..02d7811d62c 100644 --- a/src/xmpp/xmpp_connection.h +++ b/src/xmpp/xmpp_connection.h @@ -105,7 +105,6 @@ class XmppConnection { void set_session(XmppSession *session); void clear_session(); - void SetFrom(const std::string &); void SetTo(const std::string &); const XmppSession *session() const;