Skip to content

Commit 5762500

Browse files
committed
Fix[mqbblp::Domain]: reconfigure iterates queues without mutex
Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
1 parent 9ee0a6e commit 5762500

3 files changed

Lines changed: 720 additions & 24 deletions

File tree

src/groups/mqb/mqbblp/mqbblp_domain.cpp

Lines changed: 87 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,55 @@ int normalizeConfig(mqbconfm::Domain* defn,
211211
return updatedValues;
212212
}
213213

214+
/// @brief Function object that reconfigures a snapshot of queues.
215+
class QueueReconfigureFunctor {
216+
public:
217+
// TYPES
218+
219+
/// Snapshot of queues held by weak pointer.
220+
typedef bsl::vector<bsl::weak_ptr<mqbi::Queue> > QueueWeakPtrVector;
221+
222+
private:
223+
// DATA
224+
225+
/// Shared snapshot of the queues to reconfigure.
226+
bsl::shared_ptr<QueueWeakPtrVector> d_queues_sp;
227+
228+
public:
229+
// CREATORS
230+
231+
/// @brief Create a functor owning the specified `queues_sp` snapshot.
232+
///
233+
/// @param queues_sp The queues to reconfigure, held by weak pointer.
234+
explicit QueueReconfigureFunctor(
235+
const bsl::shared_ptr<QueueWeakPtrVector>& queues_sp)
236+
: d_queues_sp(queues_sp)
237+
{
238+
// PRECONDITIONS
239+
BSLS_ASSERT(queues_sp);
240+
}
241+
242+
// ACCESSORS
243+
244+
/// @brief Reconfigure every queue in the snapshot that is still alive.
245+
void operator()() const
246+
{
247+
// Thread: CLUSTER DISPATCHER
248+
249+
for (QueueWeakPtrVector::const_iterator it = d_queues_sp->begin();
250+
it != d_queues_sp->end();
251+
++it) {
252+
bsl::shared_ptr<mqbi::Queue> queue = it->lock();
253+
if (queue) {
254+
queue->configure(
255+
static_cast<bsl::ostream*>(0), // errorDescription
256+
true, // isReconfigure
257+
false); // wait
258+
}
259+
}
260+
}
261+
};
262+
214263
} // close unnamed namespace
215264

216265
// ------------
@@ -224,7 +273,7 @@ void Domain::onOpenQueueResponse(
224273
const mqbi::OpenQueueConfirmationCookieSp& confirmationCookie,
225274
const mqbi::Domain::OpenQueueCallback& callback)
226275
{
227-
// executed by *ANY* thread
276+
// Thread: CLUSTER DISPATCHER
228277

229278
--d_pendingRequests;
230279
if (status.category() == bmqp_ctrlmsg::StatusCategory::E_SUCCESS) {
@@ -287,6 +336,8 @@ Domain::~Domain()
287336
int Domain::configure(bsl::ostream& errorDescription,
288337
const mqbconfm::Domain& newConfig)
289338
{
339+
// Thread: ADMIN
340+
290341
enum RcEnum {
291342
// Value for the various RC error categories
292343
rc_SUCCESS = 0,
@@ -358,6 +409,22 @@ int Domain::configure(bsl::ostream& errorDescription,
358409
// any needed update-advisories to the CSL.
359410
d_cluster_sp->onDomainReconfigured(*this, *oldConfig, finalConfig);
360411

412+
// Make a snapshot of weak queue pointers for reconfigure.
413+
bsl::shared_ptr<QueueReconfigureFunctor::QueueWeakPtrVector>
414+
queues_sp = bsl::allocate_shared<
415+
QueueReconfigureFunctor::QueueWeakPtrVector>(d_allocator_p);
416+
{
417+
bslmt::LockGuard<bslmt::Mutex> guard(&d_mutex); // LOCK
418+
queues_sp->reserve(d_queues.size());
419+
for (QueueMapCIter it = d_queues.begin(); it != d_queues.end();
420+
++it) {
421+
queues_sp->push_back(it->second);
422+
}
423+
}
424+
425+
BALL_LOG_INFO << "Reconfiguring " << queues_sp->size()
426+
<< " queues from domain " << d_name;
427+
361428
// Note: Queues must only be reconfigured AFTER ensuring that virtual
362429
// storage has been created for any new AppIds. This is done by the
363430
// 'QueueEngine::afterAppIdRegistered' method, which is invoked on the
@@ -370,19 +437,7 @@ int Domain::configure(bsl::ostream& errorDescription,
370437
// that it happens after 'onDomainReconfigured'; since implementation
371438
// of 'Queue::configure' dispatches to the Queue dispatcher thread, it
372439
// will also happen after completion of 'afterAppIdRegistered' above.
373-
BALL_LOG_INFO << "Reconfiguring " << d_queues.size()
374-
<< " queues from domain " << d_name;
375-
376-
QueueMap::iterator it = d_queues.begin();
377-
for (; it != d_queues.end(); it++) {
378-
bsl::function<int()> reconfigureQueueFn = bdlf::BindUtil::bind(
379-
&mqbi::Queue::configure,
380-
it->second.get(),
381-
static_cast<bsl::ostream*>(0), // errorDescription_p
382-
true, // isReconfigure
383-
false); // wait
384-
d_dispatcher_p->execute(reconfigureQueueFn, cluster());
385-
}
440+
d_dispatcher_p->execute(QueueReconfigureFunctor(queues_sp), cluster());
386441
}
387442
// 'wait==false', so the result of reconfiguration is not known
388443

@@ -450,7 +505,7 @@ void Domain::openQueue(
450505
const bmqp_ctrlmsg::QueueHandleParameters& handleParameters,
451506
const mqbi::Domain::OpenQueueCallback& callback)
452507
{
453-
// will execute in DomainManager's IO requester thread
508+
// Thread: DomainManager's IO requester thread
454509
// (TBD: for now, in client-session or cluster dispatcher thread)
455510

456511
// PRECONDITIONS
@@ -502,7 +557,7 @@ void Domain::openQueue(
502557

503558
int Domain::registerQueue(const bsl::shared_ptr<mqbi::Queue>& queueSp)
504559
{
505-
// executed by the associated CLUSTER's DISPATCHER thread
560+
// Thread: CLUSTER DISPATCHER
506561

507562
// PRECONDITIONS
508563
BSLS_ASSERT_SAFE(d_cluster_sp->inDispatcherThread());
@@ -522,6 +577,8 @@ int Domain::registerQueue(const bsl::shared_ptr<mqbi::Queue>& queueSp)
522577
// invoke 'Queue.configure' outside of the lock scope, and in case it
523578
// fails, we rollback.
524579

580+
size_t count = 0;
581+
525582
{
526583
bslmt::LockGuard<bslmt::Mutex> guard(&d_mutex); // LOCK
527584

@@ -542,20 +599,22 @@ int Domain::registerQueue(const bsl::shared_ptr<mqbi::Queue>& queueSp)
542599
d_queues[queueSp->uri().queue()] = queueSp;
543600

544601
d_numQueues.add(1);
602+
603+
count = d_queues.size();
545604
}
546605

547606
BALL_LOG_INFO << "Registered queue to domain '" << d_name << "' "
548607
<< "[canonicalURI: " << queueSp->uri().canonical()
549608
<< ", qId: " << bmqp::QueueId::QueueIdInt(queueSp->id())
550609
<< "]. Total number of registered queues in the domain: "
551-
<< d_queues.size() << ".";
610+
<< count << ".";
552611

553612
return rc_SUCCESS;
554613
}
555614

556615
void Domain::unregisterQueue(mqbi::Queue* queue)
557616
{
558-
// executed by the associated CLUSTER's DISPATCHER thread
617+
// Thread: CLUSTER DISPATCHER
559618

560619
// PRECONDITIONS
561620
BSLS_ASSERT_SAFE(d_cluster_sp->inDispatcherThread());
@@ -597,7 +656,7 @@ void Domain::unregisterQueue(mqbi::Queue* queue)
597656
int Domain::processCommand(mqbcmd::DomainResult* result,
598657
const mqbcmd::DomainCommand& command)
599658
{
600-
// executed by *any* thread
659+
// Thread: ADMIN
601660

602661
if (command.isPurgeValue()) {
603662
// Some queues might be inactive. They don't have associated
@@ -662,10 +721,15 @@ int Domain::processCommand(mqbcmd::DomainResult* result,
662721
OrderedQueueMap;
663722
typedef OrderedQueueMap::const_iterator OrderedQueueMapCIter;
664723

665-
// sort by queue name
666-
OrderedQueueMap map(d_queues.cbegin(), d_queues.cend());
724+
// Make a snapshot of queues.
725+
OrderedQueueMap map(d_allocator_p);
726+
{
727+
bslmt::LockGuard<bslmt::Mutex> guard(&d_mutex); // LOCK
728+
map.insert(d_queues.cbegin(), d_queues.cend());
729+
}
730+
667731
OrderedQueueMapCIter cit;
668-
domainInfo.queueUris().reserve(d_queues.size());
732+
domainInfo.queueUris().reserve(map.size());
669733
for (cit = map.cbegin(); cit != map.cend(); ++cit) {
670734
domainInfo.queueUris().push_back(cit->second->uri().asString());
671735
}
@@ -875,7 +939,7 @@ void Domain::loadAllQueues(bsl::vector<bmqt::Uri>* out) const
875939
void Domain::loadRoutingConfiguration(
876940
bmqp_ctrlmsg::RoutingConfiguration* config) const
877941
{
878-
// executed by the associated CLUSTER's DISPATCHER thread
942+
// Thread: CLUSTER DISPATCHER
879943

880944
// PRECONDITIONS
881945
BSLS_ASSERT_SAFE(d_cluster_sp->inDispatcherThread());

src/groups/mqb/mqbblp/mqbblp_domain.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2015-2023 Bloomberg Finance L.P.
1+
// Copyright 2026 Bloomberg Finance L.P.
22
// SPDX-License-Identifier: Apache-2.0
33
//
44
// Licensed under the Apache License, Version 2.0 (the "License");

0 commit comments

Comments
 (0)