Skip to content

Commit e17270d

Browse files
RobertLauferElektrobittcormackMW
RobertLauferElektrobit
and
tcormackMW
authored
Ensure safe concurrent destruction of bundles and framework stopping (CppMicroServices#983) (CppMicroServices#1131)
Fixes bug where concurrent destruction of bundles and framework stoppage causes throws to occur Co-authored-by: tcormackMW <113473781+tcormackMW@users.noreply.github.com>
1 parent 70632b2 commit e17270d

7 files changed

Lines changed: 140 additions & 58 deletions

File tree

‎compendium/DeclarativeServices/src/manager/ComponentConfigurationImpl.cpp‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,24 @@
1-
/*=============================================================================
1+
/*=============================================================================
22
3-
Library: CppMicroServices
3+
Library: CppMicroServices
44
5-
Copyright (c) The CppMicroServices developers. See the COPYRIGHT
6-
file at the top-level directory of this distribution and at
7-
https://github.com/CppMicroServices/CppMicroServices/COPYRIGHT .
5+
Copyright (c) The CppMicroServices developers. See the COPYRIGHT
6+
file at the top-level directory of this distribution and at
7+
https://github.com/CppMicroServices/CppMicroServices/COPYRIGHT .
88
9-
Licensed under the Apache License, Version 2.0 (the "License");
10-
you may not use this file except in compliance with the License.
11-
You may obtain a copy of the License at
9+
Licensed under the Apache License, Version 2.0 (the "License");
10+
you may not use this file except in compliance with the License.
11+
You may obtain a copy of the License at
1212
13-
http://www.apache.org/licenses/LICENSE-2.0
13+
http://www.apache.org/licenses/LICENSE-2.0
1414
15-
Unless required by applicable law or agreed to in writing, software
16-
distributed under the License is distributed on an "AS IS" BASIS,
17-
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
18-
See the License for the specific language governing permissions and
19-
limitations under the License.
15+
Unless required by applicable law or agreed to in writing, software
16+
distributed under the License is distributed on an "AS IS" BASIS,
17+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
18+
See the License for the specific language governing permissions and
19+
limitations under the License.
2020
21-
=============================================================================*/
21+
=============================================================================*/
2222

2323
#include "cppmicroservices/FrameworkFactory.h"
2424

@@ -53,7 +53,7 @@ namespace cppmicroservices
5353
std::atomic<unsigned long> ComponentConfigurationImpl::idCounter(0);
5454

5555
ComponentConfigurationImpl::ComponentConfigurationImpl(
56-
std::shared_ptr<const metadata::ComponentMetadata> metadata,
56+
std::shared_ptr<metadata::ComponentMetadata const> metadata,
5757
Bundle const& bundle,
5858
std::shared_ptr<ComponentRegistry> registry,
5959
std::shared_ptr<cppmicroservices::logservice::LogService> logger,
@@ -70,7 +70,7 @@ namespace cppmicroservices
7070
, deleteCompInstanceFunc(nullptr)
7171
{
7272
if (!this->metadata || !this->bundle || !this->registry || !this->logger || !this->configNotifier)
73-
{
73+
{
7474
throw std::invalid_argument("ComponentConfigurationImpl - Invalid arguments passed to constructor");
7575
}
7676

@@ -107,7 +107,7 @@ namespace cppmicroservices
107107
std::for_each(
108108
referenceManagerTokens.begin(),
109109
referenceManagerTokens.end(),
110-
[](const std::unordered_map<std::shared_ptr<ReferenceManager>, ListenerTokenId>::value_type& kvpair)
110+
[](std::unordered_map<std::shared_ptr<ReferenceManager>, ListenerTokenId>::value_type const& kvpair)
111111
{ (kvpair.first)->UnregisterListener(kvpair.second); });
112112

113113
referenceManagerTokens.clear();
@@ -140,7 +140,7 @@ namespace cppmicroservices
140140
props.emplace(item.first, item.second);
141141
}
142142
}
143-
else
143+
else
144144
{
145145
props = metadata->properties;
146146
}
@@ -364,7 +364,7 @@ namespace cppmicroservices
364364
return (state);
365365
}
366366
void
367-
operator()(const std::unordered_map<std::string, std::shared_ptr<ReferenceManager>>::value_type& item)
367+
operator()(std::unordered_map<std::string, std::shared_ptr<ReferenceManager>>::value_type const& item)
368368
{
369369
if (skipKey.empty() || item.first != skipKey)
370370
{

‎compendium/DeclarativeServices/src/manager/ComponentManagerImpl.hpp‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -50,13 +50,13 @@ namespace cppmicroservices
5050
class ComponentManagerImpl : public ComponentManager
5151
{
5252
public:
53-
ComponentManagerImpl(std::shared_ptr<const metadata::ComponentMetadata> metadata,
53+
ComponentManagerImpl(std::shared_ptr<metadata::ComponentMetadata const> metadata,
5454
std::shared_ptr<ComponentRegistry> registry,
5555
cppmicroservices::BundleContext bundleContext,
5656
std::shared_ptr<cppmicroservices::logservice::LogService> logger,
5757
std::shared_ptr<cppmicroservices::async::AsyncWorkService> asyncWorkService,
5858
std::shared_ptr<ConfigurationNotifier> configNotifier);
59-
ComponentManagerImpl(ComponentManagerImpl const&) = delete;
59+
ComponentManagerImpl(ComponentManagerImpl const&) = delete;
6060
ComponentManagerImpl(ComponentManagerImpl&&) = delete;
6161
ComponentManagerImpl& operator=(ComponentManagerImpl const&) = delete;
6262
ComponentManagerImpl& operator=(ComponentManagerImpl&&) = delete;
@@ -90,7 +90,7 @@ namespace cppmicroservices
9090
/** @copydoc ComponentManager::GetMetadata()
9191
* Returns the stored component description
9292
*/
93-
std::shared_ptr<const metadata::ComponentMetadata>
93+
std::shared_ptr<metadata::ComponentMetadata const>
9494
GetMetadata() const override
9595
{
9696
return compDesc;
@@ -206,11 +206,11 @@ namespace cppmicroservices
206206
private:
207207
FRIEND_TEST(ComponentManagerImplParameterizedTest, TestAccumulateFutures);
208208

209-
const std::shared_ptr<ComponentRegistry>
209+
std::shared_ptr<ComponentRegistry> const
210210
registry; ///< component registry associated with the current runtime
211-
const std::shared_ptr<const metadata::ComponentMetadata> compDesc; ///< the component description
211+
std::shared_ptr<metadata::ComponentMetadata const> const compDesc; ///< the component description
212212
cppmicroservices::BundleContext bundleContext; ///< context of the bundle which contains the component
213-
const std::shared_ptr<cppmicroservices::logservice::LogService>
213+
std::shared_ptr<cppmicroservices::logservice::LogService> const
214214
logger; ///< logger associated with the current runtime
215215
std::shared_ptr<ComponentManagerState> state; ///< This member is always accessed using atomic operations
216216
std::vector<std::shared_future<void>>
@@ -221,7 +221,7 @@ namespace cppmicroservices
221221
std::mutex
222222
transitionMutex; ///< mutex to make the state transition and posting of the async operations atomic
223223
std::shared_ptr<ConfigurationNotifier> configNotifier;
224-
};
224+
};
225225
} // namespace scrimpl
226226
} // namespace cppmicroservices
227227

‎compendium/DeclarativeServices/src/manager/states/CCActiveState.cpp‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,16 @@ namespace cppmicroservices
105105
// the latch counts down to 0, thereby allowing all Activate, Rebind and Modified
106106
// activities to complete.
107107
currentState->WaitForTransitionTask(); // wait for the previous transition to finish
108-
mgr.UnregisterService();
108+
try
109+
{
110+
mgr.UnregisterService();
111+
}
112+
catch (...)
113+
{
114+
mgr.GetLogger()->Log(cppmicroservices::logservice::SeverityLevel::LOG_WARNING,
115+
"Failed to unregister the service.",
116+
std::current_exception());
117+
}
109118
mgr.DestroyComponentInstances();
110119
transitionAction.set_value();
111120
}

‎compendium/DeclarativeServices/src/manager/states/CMEnabledState.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ namespace cppmicroservices
4747
}
4848

4949
void
50-
CMEnabledState::CreateConfigurations(std::shared_ptr<const metadata::ComponentMetadata> compDesc,
50+
CMEnabledState::CreateConfigurations(std::shared_ptr<metadata::ComponentMetadata const> compDesc,
5151
cppmicroservices::Bundle const& bundle,
5252
std::shared_ptr<ComponentRegistry> registry,
5353
std::shared_ptr<logservice::LogService> logger,
@@ -60,7 +60,7 @@ namespace cppmicroservices
6060
registry,
6161
logger,
6262
configNotifier);
63-
configurations.push_back(cc);
63+
configurations.push_back(cc);
6464
}
6565
catch (cppmicroservices::SharedLibraryException const&)
6666
{

‎compendium/DeclarativeServices/test/gtest/TestComponentConfigurationImpl.cpp‎

Lines changed: 66 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ namespace cppmicroservices
128128
mockRegistry,
129129
fakeLogger,
130130
notifier);
131-
EXPECT_EQ(fakeCompConfig->GetConfigState(), ComponentState::UNSATISFIED_REFERENCE);
131+
EXPECT_EQ(fakeCompConfig->GetConfigState(), ComponentState::UNSATISFIED_REFERENCE);
132132
EXPECT_EQ(fakeCompConfig->regManager, nullptr);
133133
EXPECT_EQ(fakeCompConfig->referenceManagers.size(), static_cast<size_t>(0));
134134
});
@@ -147,9 +147,9 @@ namespace cppmicroservices
147147
auto notifier = std::make_shared<ConfigurationNotifier>(GetFramework().GetBundleContext(),
148148
fakeLogger,
149149
asyncWorkService,
150-
extRegistry);
150+
extRegistry);
151151
std::set<unsigned long> idSet;
152-
const size_t iterCount = 10;
152+
size_t const iterCount = 10;
153153
for (size_t i = 0; i < iterCount; ++i)
154154
{
155155
EXPECT_NO_THROW({
@@ -201,7 +201,7 @@ namespace cppmicroservices
201201
mockRegistry,
202202
fakeLogger,
203203
notifier);
204-
EXPECT_CALL(*fakeCompConfig, GetFactory()).Times(1).WillOnce(testing::Return(mockFactory));
204+
EXPECT_CALL(*fakeCompConfig, GetFactory()).Times(1).WillOnce(testing::Return(mockFactory));
205205
// add the mock reference managers to the config object
206206
fakeCompConfig->referenceManagers.insert(std::make_pair("ref1", refMgr1));
207207
fakeCompConfig->referenceManagers.insert(std::make_pair("ref2", refMgr2));
@@ -225,7 +225,7 @@ namespace cppmicroservices
225225
auto mockMetadata = std::make_shared<metadata::ComponentMetadata>();
226226
auto mockRegistry = std::make_shared<MockComponentRegistry>();
227227
auto fakeLogger = std::make_shared<FakeLogger>();
228-
228+
229229
auto logger = std::make_shared<SCRLogger>(GetFramework().GetBundleContext());
230230
auto asyncWorkService
231231
= std::make_shared<cppmicroservices::scrimpl::SCRAsyncWorkService>(GetFramework().GetBundleContext(),
@@ -270,7 +270,7 @@ namespace cppmicroservices
270270
auto mockMetadata = std::make_shared<metadata::ComponentMetadata>();
271271
auto mockRegistry = std::make_shared<MockComponentRegistry>();
272272
auto fakeLogger = std::make_shared<FakeLogger>();
273-
273+
274274
// Test that a call to Register with a component containing both a service
275275
// and a reference to the same service interface will not cause a state change.
276276
scrimpl::metadata::ReferenceMetadata refMetadata {};
@@ -473,7 +473,7 @@ namespace cppmicroservices
473473
auto notifier = std::make_shared<ConfigurationNotifier>(GetFramework().GetBundleContext(),
474474
fakeLogger,
475475
asyncWorkService,
476-
extRegistry);
476+
extRegistry);
477477
// Test for exception from user code
478478
auto fakeCompConfig = std::make_shared<MockComponentConfigurationImpl>(mockMetadata,
479479
GetFramework(),
@@ -710,15 +710,15 @@ namespace cppmicroservices
710710
auto notifier = std::make_shared<ConfigurationNotifier>(GetFramework().GetBundleContext(),
711711
fakeLogger,
712712
asyncWorkService,
713-
extRegistry);
713+
extRegistry);
714714
mockMetadata->serviceMetadata.interfaces = { us_service_interface_iid<dummy::ServiceImpl>() };
715715
mockMetadata->immediate = false;
716716
auto fakeCompConfig = std::make_shared<MockComponentConfigurationImpl>(mockMetadata,
717717
GetFramework(),
718718
mockRegistry,
719719
fakeLogger,
720720
notifier);
721-
EXPECT_CALL(*fakeCompConfig, GetFactory())
721+
EXPECT_CALL(*fakeCompConfig, GetFactory())
722722
.Times(testing::AtLeast(1)) // 2
723723
.WillRepeatedly(testing::Return(mockFactory));
724724
fakeCompConfig->Initialize();
@@ -843,14 +843,14 @@ namespace cppmicroservices
843843
std::make_shared<MockComponentRegistry>(),
844844
fakeLogger,
845845
notifier);
846-
846+
847847
auto fakeBundleProtoCompConfig = std::make_shared<BundleOrPrototypeComponentConfigurationImpl>(
848848
mockMetadata,
849849
GetFramework(),
850850
std::make_shared<MockComponentRegistry>(),
851851
fakeLogger,
852852
notifier);
853-
853+
854854
auto svcReg = GetFramework().GetBundleContext().RegisterService<dummy::ServiceImpl>(
855855
std::make_shared<dummy::ServiceImpl>());
856856

@@ -936,6 +936,61 @@ namespace cppmicroservices
936936
framework.WaitForStop(std::chrono::milliseconds::zero());
937937
}
938938

939+
TEST(ComponentConfigurationTests, TestConcurrentStop)
940+
{
941+
auto framework = cppmicroservices::FrameworkFactory().NewFramework();
942+
framework.Start();
943+
ASSERT_TRUE(framework);
944+
945+
auto context = framework.GetBundleContext();
946+
ASSERT_TRUE(context);
947+
948+
test::InstallAndStartDS(context);
949+
950+
std::vector<cppmicroservices::Bundle> installedBundles
951+
= { ::test::InstallAndStartBundle(context, "DSGraph01"),
952+
::test::InstallAndStartBundle(context, "DSGraph02"),
953+
::test::InstallAndStartBundle(context, "DSGraph03"),
954+
::test::InstallAndStartBundle(context, "DSGraph04"),
955+
::test::InstallAndStartBundle(context, "DSGraph05"),
956+
::test::InstallAndStartBundle(context, "DSGraph06"),
957+
::test::InstallAndStartBundle(context, "DSGraph07") };
958+
959+
std::vector<cppmicroservices::ServiceReferenceU> const interfaces {
960+
context.GetServiceReference<test::DSGraph01>(), context.GetServiceReference<test::DSGraph02>(),
961+
context.GetServiceReference<test::DSGraph03>(), context.GetServiceReference<test::DSGraph04>(),
962+
context.GetServiceReference<test::DSGraph05>(), context.GetServiceReference<test::DSGraph06>(),
963+
context.GetServiceReference<test::DSGraph07>()
964+
};
965+
966+
for (auto const& sref : interfaces)
967+
{
968+
ASSERT_TRUE(static_cast<bool>(sref));
969+
auto service = context.GetService(sref);
970+
ASSERT_NE(service, nullptr);
971+
}
972+
973+
EXPECT_NO_THROW({
974+
std::thread bundleT = std::thread(
975+
[&installedBundles]()
976+
{
977+
for (auto& bundle : installedBundles)
978+
{
979+
bundle.Stop();
980+
}
981+
});
982+
std::thread frameworkT = std::thread(
983+
[&framework]()
984+
{
985+
framework.Stop();
986+
framework.WaitForStop(std::chrono::milliseconds::zero());
987+
});
988+
989+
bundleT.join();
990+
frameworkT.join();
991+
});
992+
}
993+
939994
// Note: This is different than the other tests in this suite as Declarative Services is actually
940995
// installed and started rather than using mocks.
941996
TEST(ComponentConfigurationImplLogTest, LoadLibraryLogsMessagesNotImmediateTest)

‎framework/src/bundle/BundlePrivate.cpp‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -836,8 +836,20 @@ namespace cppmicroservices
836836
coreCtx->services.GetUsedByBundle(this, srs);
837837
for (std::vector<ServiceRegistrationBase>::const_iterator i = srs.begin(); i != srs.end(); ++i)
838838
{
839-
auto ref = i->GetReference(std::string());
840-
ref.d.Load()->UngetService(this->shared_from_this(), false);
839+
// wrap in try-catch to catch failures if service is already unregistered
840+
// if service is unregistered, all work in UngetService is already done by Unregister() previously
841+
try
842+
{
843+
auto ref = i->GetReference(std::string());
844+
ref.d.Load()->UngetService(this->shared_from_this(), false);
845+
}
846+
catch (...)
847+
{
848+
coreCtx->logger->Log(logservice::SeverityLevel::LOG_WARNING,
849+
"Some services already unregistered in Bundle " + symbolicName
850+
+ " (location=" + location + ")",
851+
std::current_exception());
852+
}
841853
}
842854
}
843855

0 commit comments

Comments
 (0)