Skip to content

Commit

Permalink
Fix check-then-act race for BundleContext (#665)
Browse files Browse the repository at this point in the history 
* Added test case

Signed-off-by: The MathWorks, Inc. <alchrist@mathworks.com>

* Fix check-then-act race for GetServiceFromFactory

Signed-off-by: The MathWorks, Inc. <alchrist@mathworks.com>

* Fixed test names and cleaned up tests

Signed-off-by: The MathWorks, Inc. <alchrist@mathworks.com>

* More cleanup

* Fixed CTA races in BundleContext

Signed-off-by: The MathWorks, Inc. <alchrist@mathworks.com>

* Made changes requested by reviewers
  • Loading branch information
@achristoforides
achristoforides committed May 11, 2022
1 parent d11c608 commit 1e11077
Show file tree
Hide file tree
Showing 8 changed files with 104 additions and 178 deletions.
205 changes: 53 additions & 152 deletions framework/src/bundle/BundleContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,19 @@

namespace cppmicroservices {

namespace {
std::shared_ptr<BundlePrivate> GetAndCheckBundlePrivate(
const std::shared_ptr<BundleContextPrivate>& d)
{
auto b = (d->Lock(), d->bundle.lock());
if (!b) {
throw std::runtime_error("The bundle context is no longer valid");
}

return b;
}
}

BundleContext::BundleContext(std::shared_ptr<BundleContextPrivate> ctx)
: d(std::move(ctx))
{}
Expand Down Expand Up @@ -79,7 +92,12 @@ std::shared_ptr<detail::LogSink> BundleContext::GetLogSink() const
}

d->CheckValid();
return d->bundle->coreCtx->sink->shared_from_this();

if (auto bundle_ = d->bundle.lock()) {
return bundle_->coreCtx->sink->shared_from_this();
} else {
throw std::runtime_error("The bundle context is no longer valid");
}
}

Any BundleContext::GetProperty(const std::string& key) const
Expand All @@ -89,12 +107,7 @@ Any BundleContext::GetProperty(const std::string& key) const
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

auto iter = b->coreCtx->frameworkProperties.find(key);
return iter == b->coreCtx->frameworkProperties.end() ? Any() : iter->second;
Expand All @@ -107,12 +120,7 @@ AnyMap BundleContext::GetProperties() const
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

return b->coreCtx->frameworkProperties;
}
Expand All @@ -124,14 +132,9 @@ Bundle BundleContext::GetBundle() const
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

return MakeBundle(b->shared_from_this());
return MakeBundle(b);
}

Bundle BundleContext::GetBundle(long id) const
Expand All @@ -141,12 +144,7 @@ Bundle BundleContext::GetBundle(long id) const
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

return b->coreCtx->bundleHooks.FilterBundle(
*this, MakeBundle(b->coreCtx->bundleRegistry.GetBundle(id)));
Expand All @@ -159,12 +157,7 @@ std::vector<Bundle> BundleContext::GetBundles(const std::string& location) const
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

std::vector<Bundle> res;
for (auto bu : b->coreCtx->bundleRegistry.GetBundles(location)) {
Expand All @@ -180,12 +173,7 @@ std::vector<Bundle> BundleContext::GetBundles() const
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

std::vector<Bundle> bus;
for (auto bu : b->coreCtx->bundleRegistry.GetBundles()) {
Expand All @@ -204,14 +192,9 @@ ServiceRegistrationU BundleContext::RegisterService(
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

return b->coreCtx->services.RegisterService(b, service, properties);
return b->coreCtx->services.RegisterService(b.get(), service, properties);
}

std::vector<ServiceReferenceU> BundleContext::GetServiceReferences(
Expand All @@ -223,15 +206,10 @@ std::vector<ServiceReferenceU> BundleContext::GetServiceReferences(
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

std::vector<ServiceReferenceBase> refs;
b->coreCtx->services.Get(clazz, filter, b, refs);
b->coreCtx->services.Get(clazz, filter, b.get(), refs);
return std::vector<ServiceReferenceU>(refs.begin(), refs.end());
}

Expand All @@ -242,14 +220,9 @@ ServiceReferenceU BundleContext::GetServiceReference(const std::string& clazz)
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

return b->coreCtx->services.Get(d->bundle, clazz);
return b->coreCtx->services.Get(b.get(), clazz);
}

/* @brief Private helper struct used to facilitate the shared_ptr aliasing constructor
Expand Down Expand Up @@ -308,15 +281,10 @@ std::shared_ptr<void> BundleContext::GetService(
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

std::shared_ptr<ServiceHolder<void>> h(new ServiceHolder<void>(
b->shared_from_this(), reference, reference.d.load()->GetService(b)));
b, reference, reference.d.load()->GetService(b.get())));
return std::shared_ptr<void>(h, h->service.get());
}

Expand All @@ -333,16 +301,12 @@ InterfaceMapConstPtr BundleContext::GetService(
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);
auto b = GetAndCheckBundlePrivate(d);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto serviceInterfaceMap = reference.d.load()->GetServiceInterfaceMap(b);
auto serviceInterfaceMap =
reference.d.load()->GetServiceInterfaceMap(b.get());
std::shared_ptr<ServiceHolder<const InterfaceMap>> h(
new ServiceHolder<const InterfaceMap>(
b->shared_from_this(), reference, serviceInterfaceMap));
new ServiceHolder<const InterfaceMap>(b, reference, serviceInterfaceMap));
return InterfaceMapConstPtr(h, h->service.get());
}

Expand All @@ -354,12 +318,7 @@ ListenerToken BundleContext::AddServiceListener(const ServiceListener& delegate,
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

return b->coreCtx->listeners.AddServiceListener(d, delegate, nullptr, filter);
}
Expand All @@ -371,12 +330,7 @@ void BundleContext::RemoveServiceListener(const ServiceListener& delegate)
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

b->coreCtx->listeners.RemoveServiceListener(
d, ListenerTokenId(0), delegate, nullptr);
Expand All @@ -389,12 +343,7 @@ ListenerToken BundleContext::AddBundleListener(const BundleListener& delegate)
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

return b->coreCtx->listeners.AddBundleListener(d, delegate, nullptr);
}
Expand All @@ -406,12 +355,7 @@ void BundleContext::RemoveBundleListener(const BundleListener& delegate)
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

b->coreCtx->listeners.RemoveBundleListener(d, delegate, nullptr);
}
Expand All @@ -424,12 +368,7 @@ ListenerToken BundleContext::AddFrameworkListener(
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

return b->coreCtx->listeners.AddFrameworkListener(d, listener, nullptr);
}
Expand All @@ -441,12 +380,7 @@ void BundleContext::RemoveFrameworkListener(const FrameworkListener& listener)
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

b->coreCtx->listeners.RemoveFrameworkListener(d, listener, nullptr);
}
Expand All @@ -460,12 +394,7 @@ ListenerToken BundleContext::AddServiceListener(const ServiceListener& delegate,
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

return b->coreCtx->listeners.AddServiceListener(d, delegate, data, filter);
}
Expand All @@ -478,12 +407,7 @@ void BundleContext::RemoveServiceListener(const ServiceListener& delegate,
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

b->coreCtx->listeners.RemoveServiceListener(
d, ListenerTokenId(0), delegate, data);
Expand All @@ -497,12 +421,7 @@ ListenerToken BundleContext::AddBundleListener(const BundleListener& delegate,
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

return b->coreCtx->listeners.AddBundleListener(d, delegate, data);
}
Expand All @@ -515,12 +434,7 @@ void BundleContext::RemoveBundleListener(const BundleListener& delegate,
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

b->coreCtx->listeners.RemoveBundleListener(d, delegate, data);
}
Expand All @@ -532,12 +446,7 @@ void BundleContext::RemoveListener(ListenerToken token)
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

b->coreCtx->listeners.RemoveListener(d, std::move(token));
}
Expand All @@ -549,12 +458,7 @@ std::string BundleContext::GetDataFile(const std::string& filename) const
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);

// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
auto b = GetAndCheckBundlePrivate(d);

std::string dataRoot = b->bundleDir;
if (!dataRoot.empty()) {
Expand All @@ -575,12 +479,9 @@ std::vector<Bundle> BundleContext::InstallBundles(
}

d->CheckValid();
auto b = (d->Lock(), d->bundle);
// CONCURRENCY NOTE: This is a check-then-act situation,
// but we ignore it since the time window is small and
// the result is the same as if the calling thread had
// won the race condition.
return b->coreCtx->bundleRegistry.Install(location, b, bundleManifest);
auto b = GetAndCheckBundlePrivate(d);

return b->coreCtx->bundleRegistry.Install(location, b.get(), bundleManifest);
}

}

0 comments on commit 1e11077

Please sign in to comment.