table: don't share Strategy instance among StrategyChoice entries
For a Strategy type registered in the strategy registry,
a new instance is created for each StrategyChoice entry that uses it.
StrategyChoice::install is deprecated. An installed strategy instance
is still shared among StrategyChoice entries that use it. This will
be removed after unit tests switch to use the strategy registry.
refs #3868
Change-Id: Ibca685e6b6668f64fa1a503e3575867e8babdfe1
diff --git a/daemon/fw/access-strategy.cpp b/daemon/fw/access-strategy.cpp
index ace5426..45235d8 100644
--- a/daemon/fw/access-strategy.cpp
+++ b/daemon/fw/access-strategy.cpp
@@ -230,6 +230,7 @@
AccessStrategy::updateMeasurements(const Face& inFace, const Data& data,
const RttEstimator::Duration& rtt)
{
+ ///\todo move FaceInfoTable out of AccessStrategy instance, to Measurements or somewhere else
FaceInfo& fi = m_fit[inFace.getId()];
fi.rtt.addMeasurement(rtt);
diff --git a/daemon/fw/forwarder.cpp b/daemon/fw/forwarder.cpp
index 7482cf4..8efc522 100644
--- a/daemon/fw/forwarder.cpp
+++ b/daemon/fw/forwarder.cpp
@@ -68,7 +68,6 @@
});
m_strategyChoice.setDefaultStrategy(getDefaultStrategyName());
- m_strategyChoice.installFromRegistry();
}
Forwarder::~Forwarder() = default;
diff --git a/daemon/table/strategy-choice-entry.cpp b/daemon/table/strategy-choice-entry.cpp
index 365fd20..f5d9551 100644
--- a/daemon/table/strategy-choice-entry.cpp
+++ b/daemon/table/strategy-choice-entry.cpp
@@ -32,15 +32,35 @@
Entry::Entry(const Name& prefix)
: m_prefix(prefix)
- , m_strategy(nullptr)
+ , m_strategyPtr(nullptr)
, m_nameTreeEntry(nullptr)
{
}
+Entry::~Entry() = default;
+
const Name&
Entry::getStrategyName() const
{
- return m_strategy->getName();
+ return this->getStrategy().getName();
+}
+
+void
+Entry::setStrategy(fw::Strategy& strategy)
+{
+ /// \todo #3868
+ /// Every entry should have its own Strategy instance in unique_ptr;
+ /// m_strategyPtr and this overload should be eliminated.
+
+ m_strategy.reset();
+ m_strategyPtr = &strategy;
+}
+
+void
+Entry::setStrategy(unique_ptr<fw::Strategy> strategy)
+{
+ m_strategy = std::move(strategy);
+ m_strategyPtr = m_strategy.get();
}
} // namespace strategy_choice
diff --git a/daemon/table/strategy-choice-entry.hpp b/daemon/table/strategy-choice-entry.hpp
index c63676b..5d1497c 100644
--- a/daemon/table/strategy-choice-entry.hpp
+++ b/daemon/table/strategy-choice-entry.hpp
@@ -47,6 +47,8 @@
public:
Entry(const Name& prefix);
+ ~Entry();
+
const Name&
getPrefix() const
{
@@ -59,19 +61,21 @@
fw::Strategy&
getStrategy() const
{
- BOOST_ASSERT(m_strategy != nullptr);
- return *m_strategy;
+ BOOST_ASSERT(m_strategyPtr != nullptr);
+ return *m_strategyPtr;
}
+ DEPRECATED(
void
- setStrategy(fw::Strategy& strategy)
- {
- m_strategy = &strategy;
- }
+ setStrategy(fw::Strategy& strategy));
+
+ void
+ setStrategy(unique_ptr<fw::Strategy> strategy);
private:
Name m_prefix;
- fw::Strategy* m_strategy;
+ unique_ptr<fw::Strategy> m_strategy;
+ fw::Strategy* m_strategyPtr;
name_tree::Entry* m_nameTreeEntry;
diff --git a/daemon/table/strategy-choice.cpp b/daemon/table/strategy-choice.cpp
index 81de636..985d05c 100644
--- a/daemon/table/strategy-choice.cpp
+++ b/daemon/table/strategy-choice.cpp
@@ -52,38 +52,15 @@
void
StrategyChoice::setDefaultStrategy(const Name& strategyName)
{
- unique_ptr<Strategy> strategy = Strategy::create(strategyName, m_forwarder);
-
- bool isInstalled = false;
- Strategy* instance = nullptr;
- std::tie(isInstalled, instance) = this->install(std::move(strategy));
- BOOST_ASSERT(isInstalled);
-
auto entry = make_unique<Entry>(Name());
- entry->setStrategy(*instance);
+ entry->setStrategy(Strategy::create(strategyName, m_forwarder));
+ NFD_LOG_INFO("setDefaultStrategy " << entry->getStrategyName());
// don't use .insert here, because it will invoke findEffectiveStrategy
// which expects an existing root entry
name_tree::Entry& nte = m_nameTree.lookup(Name());
nte.setStrategyChoiceEntry(std::move(entry));
++m_nItems;
- NFD_LOG_INFO("setDefaultStrategy " << instance->getName());
-}
-
-void
-StrategyChoice::installFromRegistry()
-{
- /// \todo #3868
- /// * Give every StrategyChoice entry its own Strategy instance.
- /// Don't share Strategy instances.
- /// * Create Strategy instance with Strategy::create.
- /// * Eliminate install, installFromRegistry, getStrategy functions.
-
- // This is temporary code until the above is done.
-
- for (const Name& strategyName : Strategy::listRegistered()) {
- this->install(Strategy::create(strategyName, m_forwarder));
- }
}
bool
@@ -93,13 +70,18 @@
return m_strategyInstances.count(strategyName) > 0;
}
else {
- return this->getStrategy(strategyName) != nullptr;
+ return Strategy::canCreate(strategyName) ||
+ this->getStrategy(strategyName) != nullptr;
}
}
std::pair<bool, Strategy*>
StrategyChoice::install(unique_ptr<Strategy> strategy)
{
+ /// \todo #3868
+ /// Strategy instance should always be created from Strategy::create;
+ /// m_strategyInstances, install, getStrategy, hasStrategy should be eliminated.
+
BOOST_ASSERT(strategy != nullptr);
Name strategyName = strategy->getName();
// copying Name, so that strategyName remains available even if strategy is deallocated
@@ -134,17 +116,22 @@
bool
StrategyChoice::insert(const Name& prefix, const Name& strategyName)
{
- Strategy* strategy = this->getStrategy(strategyName);
+ unique_ptr<Strategy> createdStrategy = Strategy::create(strategyName, m_forwarder);
+ Strategy* strategy = createdStrategy.get();
if (strategy == nullptr) {
- NFD_LOG_ERROR("insert(" << prefix << "," << strategyName << ") strategy not installed");
- return false;
+ strategy = this->getStrategy(strategyName);
+ if (strategy == nullptr) {
+ NFD_LOG_ERROR("insert(" << prefix << "," << strategyName << ") strategy not registered or installed");
+ return false;
+ }
+ NFD_LOG_WARN("insert(" << prefix << "," << strategyName << ") using shared strategy instance");
}
name_tree::Entry& nte = m_nameTree.lookup(prefix);
Entry* entry = nte.getStrategyChoiceEntry();
Strategy* oldStrategy = nullptr;
if (entry != nullptr) {
- if (entry->getStrategy().getName() == strategy->getName()) {
+ if (entry->getStrategyName() == strategy->getName()) {
NFD_LOG_TRACE("insert(" << prefix << ") not changing " << strategy->getName());
return true;
}
@@ -152,8 +139,7 @@
NFD_LOG_TRACE("insert(" << prefix << ") changing from " << oldStrategy->getName() <<
" to " << strategy->getName());
}
-
- if (entry == nullptr) {
+ else {
oldStrategy = &this->findEffectiveStrategy(prefix);
auto newEntry = make_unique<Entry>(prefix);
entry = newEntry.get();
@@ -163,7 +149,12 @@
}
this->changeStrategy(*entry, *oldStrategy, *strategy);
- entry->setStrategy(*strategy);
+ if (createdStrategy != nullptr) {
+ entry->setStrategy(std::move(createdStrategy));
+ }
+ else {
+ entry->setStrategy(*strategy);
+ }
return true;
}
diff --git a/daemon/table/strategy-choice.hpp b/daemon/table/strategy-choice.hpp
index f2410e2..2c492cd 100644
--- a/daemon/table/strategy-choice.hpp
+++ b/daemon/table/strategy-choice.hpp
@@ -67,26 +67,26 @@
void
setDefaultStrategy(const Name& strategyName);
- /** \brief install all strategies from registry
- */
- void
- installFromRegistry();
-
/** \brief determines if a strategy is installed
* \param strategyName name of the strategy
* \param isExact true to require exact match, false to permit unversioned strategyName
* \return true if strategy is installed
*/
+ DEPRECATED(
bool
- hasStrategy(const Name& strategyName, bool isExact = false) const;
+ hasStrategy(const Name& strategyName, bool isExact = false) const);
+
+ // DEPRECATED macro does not work when this type appears inline on install function.
+ typedef std::pair<bool, fw::Strategy*> InstallResult;
/** \brief install a strategy
* \return if installed, true, and a pointer to the strategy instance;
* if not installed due to duplicate strategyName, false,
* and a pointer to the existing strategy instance
*/
- std::pair<bool, fw::Strategy*>
- install(unique_ptr<fw::Strategy> strategy);
+ DEPRECATED(
+ InstallResult
+ install(unique_ptr<fw::Strategy> strategy));
public: // Strategy Choice table
/** \brief set strategy of prefix to be strategyName