fix(AbstractEvent): operator-= holds the event mutex across Delegate:…#5358
Merged
Conversation
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.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates event delegate removal/clear to avoid holding the event mutex while disabling delegates (addressing a TSAN-reported lock-order cycle), and adds a targeted test case.
Changes:
- Refactors
AbstractEvent::operator-=andAbstractEvent::clear()to detach delegates under the event mutex and calldisable()after releasing it. - Extends notification strategies (
DefaultStrategy,PriorityStrategy) withdetach*helpers to support the new lock-breaking behavior. - Adds a new
BasicEventTestcovering delegate removal duringnotify(); adjusts an environment multithread test (but introduces a capture mismatch).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Foundation/testsuite/src/CoreTest.cpp | Adjusts thread lambda capture lists in testEnvironmentMultiThread() (currently breaks compilation). |
| Foundation/testsuite/src/BasicEventTest.h | Declares new test testRemoveDelegateFromNotify(). |
| Foundation/testsuite/src/BasicEventTest.cpp | Adds cross-removal helper and new test documenting/targeting the lock-order cycle. |
| Foundation/include/Poco/PriorityStrategy.h | Adds detach, detachAll and updates remove/clear to disable after detaching. |
| Foundation/include/Poco/DefaultStrategy.h | Adds documented detach, detachAll and updates remove/clear similarly. |
| Foundation/include/Poco/AbstractEvent.h | Uses detach/detachAll so disable() runs outside the event mutex. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…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.
matejk
pushed a commit
that referenced
this pull request
May 20, 2026
#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.
matejk
added a commit
that referenced
this pull request
May 20, 2026
Four PRs merged on main after the release commit and cherry-picked into poco-1.15.3: - #5358 fix(AbstractEvent): operator-= holds the event mutex across Delegate destructor - #5359 enh(MongoDB): MongoDB 6.0/7.0/8.0 compat (carries its own API Changes block for SCRAM-SHA-256 default, count() via $count, deprecated INDEX_BACKGROUND / CMD_MAP_REDUCE) - #5355 (GH #5354) feat(CppUnit): terminate handler - #5356 fix(Foundation): gate atomic_flag::test on C++20 atomic_wait Also tighten the Summary paragraph to call out the MongoDB compat work and add the MongoDB API Changes to doc/99100-ReleaseNotes.page to match CHANGELOG. CONTRIBUTORS unchanged -- all four PR authors (aleks-f, uilianries, matejk) are already listed.
matejk
added a commit
that referenced
this pull request
May 20, 2026
…5.3 overview Four PRs merged on main after the release commit and cherry-picked into poco-1.15.3: - #5358 fix(AbstractEvent): operator-= holds the event mutex across Delegate destructor - #5359 enh(MongoDB): MongoDB 6.0/7.0/8.0 compat (carries its own API Changes for SCRAM-SHA-256 default, count() via $count, deprecated INDEX_BACKGROUND / CMD_MAP_REDUCE) - #5355 (GH #5354) feat(CppUnit): terminate handler - #5356 fix(Foundation): gate atomic_flag::test on C++20 atomic_wait Also tighten the 1.15.3 entries: trim the Summary paragraph, replace multi-paragraph API Changes prose with one-line bullets, drop redundant qualifiers from bug-fix / enhancement bullets, and use the verbatim GitHub issue/PR titles for each item. CHANGELOG and doc/99100-ReleaseNotes.page now carry the same content. CONTRIBUTORS unchanged -- all four PR authors (aleks-f, uilianries, matejk) are already listed.
matejk
added a commit
that referenced
this pull request
May 20, 2026
…5.3 overview Four PRs merged on main after the release commit and cherry-picked into poco-1.15.3: - #5358 fix(AbstractEvent): operator-= holds the event mutex across Delegate destructor - #5359 enh(MongoDB): MongoDB 6.0/7.0/8.0 compat (carries its own API Changes for SCRAM-SHA-256 default, count() via $count, deprecated INDEX_BACKGROUND / CMD_MAP_REDUCE) - #5355 (GH #5354) feat(CppUnit): terminate handler - #5356 fix(Foundation): gate atomic_flag::test on C++20 atomic_wait Also tighten the 1.15.3 entries: trim the Summary paragraph, replace multi-paragraph API Changes prose with one-line bullets, drop redundant qualifiers from bug-fix / enhancement bullets, and use the verbatim GitHub issue/PR titles for each item. CHANGELOG and doc/99100-ReleaseNotes.page now carry the same content. CONTRIBUTORS unchanged -- all four PR authors (aleks-f, uilianries, matejk) are already listed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…:disable #5357