Skip to content

Commit 9e8c5c1

Browse files
aleks-fmatejk
aleks-f
authored and
matejk
committed
fix(AbstractEvent): operator-= holds the event mutex across Delegate:… (#5358)
* fix(AbstractEvent): operator-= holds the event mutex across Delegate::disable #5357 * chore(test): drop redundant lambda captures in CoreTest CoreTest::testEnvironmentMultiThread captured const int locals (iterations, numWriters) and an unused id in the reader lambda. Clang flagged the const captures as not required and the id capture as unused. Remove them; the lambda bodies still resolve those names from the enclosing scope without explicit capture. * fix(Foundation): add detach/detachAll to NotificationStrategy concept #5358 AbstractEvent's lock-order fix calls _strategy.detach() and uses TStrategy::DelegatePtr, which were not part of the documented NotificationStrategy interface. Custom downstream strategies would fail with a confusing per-call-site error instead of an interface mismatch. Promote detach/detachAll and the DelegatePtr/Delegates typedefs to NotificationStrategy as pure virtual to make the new requirement explicit. In-tree strategies (DefaultStrategy, PriorityStrategy, FIFOStrategy) already implement them. Also strengthen testRemoveDelegateFromNotify with assertEqual(0, b.fired()) so the documented behaviour (delegate removed during notify is not invoked later in the same sequence) is validated even without TSAN.
1 parent 85a9df4 commit 9e8c5c1

7 files changed

Lines changed: 313 additions & 42 deletions

File tree

‎Foundation/include/Poco/AbstractEvent.h‎

Lines changed: 51 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,16 @@ class AbstractEvent
184184
///
185185
/// If the delegate is not found, this function does nothing.
186186
{
187-
typename TMutex::ScopedLock lock(_mutex);
188-
_strategy.remove(aDelegate);
187+
// Detach under the event mutex, disable() outside it. Holding
188+
// the event mutex across Delegate::disable() would close a
189+
// lock-order cycle with the M_delegate -> M_event edge that
190+
// notify()'s target callbacks set up when they do `event -= d`.
191+
typename TStrategy::DelegatePtr pRemoved;
192+
{
193+
typename TMutex::ScopedLock lock(_mutex);
194+
pRemoved = _strategy.detach(aDelegate);
195+
}
196+
if (pRemoved) pRemoved->disable();
189197
}
190198

191199
DelegateHandle add(const TDelegate& aDelegate)
@@ -206,8 +214,12 @@ class AbstractEvent
206214
///
207215
/// If the delegate is not found, this function does nothing.
208216
{
209-
typename TMutex::ScopedLock lock(_mutex);
210-
_strategy.remove(delegateHandle);
217+
typename TStrategy::DelegatePtr pRemoved;
218+
{
219+
typename TMutex::ScopedLock lock(_mutex);
220+
pRemoved = _strategy.detach(delegateHandle);
221+
}
222+
if (pRemoved) pRemoved->disable();
211223
}
212224

213225
void operator () (const void* pSender, TArgs& args)
@@ -305,8 +317,17 @@ class AbstractEvent
305317
void clear()
306318
/// Removes all delegates.
307319
{
308-
typename TMutex::ScopedLock lock(_mutex);
309-
_strategy.clear();
320+
// Same reason as operator -=: disable() must run outside _mutex.
321+
typename TStrategy::Delegates detached;
322+
{
323+
typename TMutex::ScopedLock lock(_mutex);
324+
detached = _strategy.detachAll();
325+
}
326+
for (typename TStrategy::Delegates::iterator it = detached.begin();
327+
it != detached.end(); ++it)
328+
{
329+
(*it)->disable();
330+
}
310331
}
311332

312333
bool empty() const
@@ -389,8 +410,13 @@ class AbstractEvent<void, TStrategy, TDelegate, TMutex>
389410
///
390411
/// If the delegate is not found, this function does nothing.
391412
{
392-
typename TMutex::ScopedLock lock(_mutex);
393-
_strategy.remove(aDelegate);
413+
// See the TArgs specialization for the rationale.
414+
typename TStrategy::DelegatePtr pRemoved;
415+
{
416+
typename TMutex::ScopedLock lock(_mutex);
417+
pRemoved = _strategy.detach(aDelegate);
418+
}
419+
if (pRemoved) pRemoved->disable();
394420
}
395421

396422
DelegateHandle add(const TDelegate& aDelegate)
@@ -411,8 +437,12 @@ class AbstractEvent<void, TStrategy, TDelegate, TMutex>
411437
///
412438
/// If the delegate is not found, this function does nothing.
413439
{
414-
typename TMutex::ScopedLock lock(_mutex);
415-
_strategy.remove(delegateHandle);
440+
typename TStrategy::DelegatePtr pRemoved;
441+
{
442+
typename TMutex::ScopedLock lock(_mutex);
443+
pRemoved = _strategy.detach(delegateHandle);
444+
}
445+
if (pRemoved) pRemoved->disable();
416446
}
417447

418448
void operator () (const void* pSender)
@@ -503,8 +533,17 @@ class AbstractEvent<void, TStrategy, TDelegate, TMutex>
503533
void clear()
504534
/// Removes all delegates.
505535
{
506-
typename TMutex::ScopedLock lock(_mutex);
507-
_strategy.clear();
536+
// Same reason as operator -=: disable() must run outside _mutex.
537+
typename TStrategy::Delegates detached;
538+
{
539+
typename TMutex::ScopedLock lock(_mutex);
540+
detached = _strategy.detachAll();
541+
}
542+
for (typename TStrategy::Delegates::iterator it = detached.begin();
543+
it != detached.end(); ++it)
544+
{
545+
(*it)->disable();
546+
}
508547
}
509548

510549
bool empty() const

‎Foundation/include/Poco/DefaultStrategy.h‎

Lines changed: 66 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -66,29 +66,59 @@ class DefaultStrategy: public NotificationStrategy<TArgs, TDelegate>
6666
}
6767

6868
void remove(const TDelegate& delegate)
69+
{
70+
DelegatePtr p = detach(delegate);
71+
if (p) p->disable();
72+
}
73+
74+
void remove(DelegateHandle delegateHandle)
75+
{
76+
DelegatePtr p = detach(delegateHandle);
77+
if (p) p->disable();
78+
}
79+
80+
DelegatePtr detach(const TDelegate& delegate)
81+
/// Removes a matching delegate from the internal list without
82+
/// disabling it, and returns the detached delegate (or an empty
83+
/// DelegatePtr if no match). The caller is responsible for
84+
/// calling disable() on the returned delegate. This separation
85+
/// allows AbstractEvent to call disable() outside its own mutex,
86+
/// breaking the lock-order edge M_event -> M_delegate.
6987
{
7088
for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
7189
{
7290
if (delegate.equals(**it))
7391
{
74-
(*it)->disable();
92+
DelegatePtr p = *it;
7593
_delegates.erase(it);
76-
return;
94+
return p;
7795
}
7896
}
97+
return DelegatePtr();
7998
}
8099

81-
void remove(DelegateHandle delegateHandle)
100+
DelegatePtr detach(DelegateHandle delegateHandle)
82101
{
83102
for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
84103
{
85104
if (*it == delegateHandle)
86105
{
87-
(*it)->disable();
106+
DelegatePtr p = *it;
88107
_delegates.erase(it);
89-
return;
108+
return p;
90109
}
91110
}
111+
return DelegatePtr();
112+
}
113+
114+
Delegates detachAll()
115+
/// Removes all delegates from the internal list without disabling
116+
/// them, and returns them so the caller can call disable() outside
117+
/// its mutex. See detach().
118+
{
119+
Delegates out;
120+
out.swap(_delegates);
121+
return out;
92122
}
93123

94124
DefaultStrategy& operator = (const DefaultStrategy& s)
@@ -102,11 +132,11 @@ class DefaultStrategy: public NotificationStrategy<TArgs, TDelegate>
102132

103133
void clear()
104134
{
105-
for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
135+
Delegates detached = detachAll();
136+
for (Iterator it = detached.begin(); it != detached.end(); ++it)
106137
{
107138
(*it)->disable();
108139
}
109-
_delegates.clear();
110140
}
111141

112142
bool empty() const
@@ -164,29 +194,51 @@ class DefaultStrategy<void,TDelegate>: public NotificationStrategy<void, TDelega
164194
}
165195

166196
void remove(const TDelegate& delegate)
197+
{
198+
DelegatePtr p = detach(delegate);
199+
if (p) p->disable();
200+
}
201+
202+
void remove(DelegateHandle delegateHandle)
203+
{
204+
DelegatePtr p = detach(delegateHandle);
205+
if (p) p->disable();
206+
}
207+
208+
DelegatePtr detach(const TDelegate& delegate)
209+
/// See the TArgs specialization above.
167210
{
168211
for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
169212
{
170213
if (delegate.equals(**it))
171214
{
172-
(*it)->disable();
215+
DelegatePtr p = *it;
173216
_delegates.erase(it);
174-
return;
217+
return p;
175218
}
176219
}
220+
return DelegatePtr();
177221
}
178222

179-
void remove(DelegateHandle delegateHandle)
223+
DelegatePtr detach(DelegateHandle delegateHandle)
180224
{
181225
for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
182226
{
183227
if (*it == delegateHandle)
184228
{
185-
(*it)->disable();
229+
DelegatePtr p = *it;
186230
_delegates.erase(it);
187-
return;
231+
return p;
188232
}
189233
}
234+
return DelegatePtr();
235+
}
236+
237+
Delegates detachAll()
238+
{
239+
Delegates out;
240+
out.swap(_delegates);
241+
return out;
190242
}
191243

192244
DefaultStrategy& operator = (const DefaultStrategy& s)
@@ -209,11 +261,11 @@ class DefaultStrategy<void,TDelegate>: public NotificationStrategy<void, TDelega
209261

210262
void clear()
211263
{
212-
for (Iterator it = _delegates.begin(); it != _delegates.end(); ++it)
264+
Delegates detached = detachAll();
265+
for (Iterator it = detached.begin(); it != detached.end(); ++it)
213266
{
214267
(*it)->disable();
215268
}
216-
_delegates.clear();
217269
}
218270

219271
bool empty() const

‎Foundation/include/Poco/NotificationStrategy.h‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919

2020

2121
#include "Poco/Foundation.h"
22+
#include "Poco/SharedPtr.h"
23+
#include <vector>
2224

2325

2426
namespace Poco {
@@ -34,6 +36,8 @@ class NotificationStrategy
3436
{
3537
public:
3638
using DelegateHandle = TDelegate*;
39+
using DelegatePtr = SharedPtr<TDelegate>;
40+
using Delegates = std::vector<DelegatePtr>;
3741

3842
NotificationStrategy() = default;
3943

@@ -53,6 +57,23 @@ class NotificationStrategy
5357
/// Removes a delegate from the strategy, if found.
5458
/// Does nothing if the delegate has not been added.
5559

60+
virtual DelegatePtr detach(const TDelegate& delegate) = 0;
61+
/// Removes a matching delegate from the strategy without disabling
62+
/// it, and returns the detached delegate (or an empty DelegatePtr
63+
/// if no match). The caller is responsible for calling disable()
64+
/// on the returned delegate. AbstractEvent uses this to avoid
65+
/// holding the event mutex across Delegate::disable() — see
66+
/// AbstractEvent::operator -= for the lock-order rationale.
67+
68+
virtual DelegatePtr detach(DelegateHandle delegateHandle) = 0;
69+
/// Same as detach(const TDelegate&), keyed by handle.
70+
71+
virtual Delegates detachAll() = 0;
72+
/// Removes all delegates from the strategy without disabling them
73+
/// and returns them, so the caller can disable() each outside its
74+
/// own mutex. AbstractEvent::clear() uses this for the same reason
75+
/// detach() exists.
76+
5677
virtual void clear() = 0;
5778
/// Removes all delegates from the strategy.
5879

@@ -71,6 +92,8 @@ class NotificationStrategy<void, TDelegate>
7192
{
7293
public:
7394
using DelegateHandle = TDelegate*;
95+
using DelegatePtr = SharedPtr<TDelegate>;
96+
using Delegates = std::vector<DelegatePtr>;
7497

7598
NotificationStrategy() = default;
7699

@@ -90,6 +113,15 @@ class NotificationStrategy<void, TDelegate>
90113
/// Removes a delegate from the strategy, if found.
91114
/// Does nothing if the delegate has not been added.
92115

116+
virtual DelegatePtr detach(const TDelegate& delegate) = 0;
117+
/// See the TArgs specialization above.
118+
119+
virtual DelegatePtr detach(DelegateHandle delegateHandle) = 0;
120+
/// See the TArgs specialization above.
121+
122+
virtual Delegates detachAll() = 0;
123+
/// See the TArgs specialization above.
124+
93125
virtual void clear() = 0;
94126
/// Removes all delegates from the strategy.
95127

0 commit comments

Comments
 (0)