Skip to content

Test AtomicSharedPtr concurrency - #13566

Open
bneradt wants to merge 1 commit into
apache:masterfrom
bneradt:test-atomic-shared-ptr
Open

Test AtomicSharedPtr concurrency#13566
bneradt wants to merge 1 commit into
apache:masterfrom
bneradt:test-atomic-shared-ptr

Conversation

@bneradt

@bneradt bneradt commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Add Catch2 coverage for AtomicSharedPtr load/store/exchange operations and concurrent readers during writer swaps. Also clarify the fallback implementation comment so it no longer relies on stale library-version shorthand.

Copilot AI lite review requested due to automatic review settings August 18, 2026 20:27
@bneradt bneradt added this to the 11.0.0 milestone Aug 18, 2026
@bneradt bneradt self-assigned this Aug 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@bneradt bneradt added the Tests label Aug 18, 2026
@bneradt

bneradt commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Sanitizer validation completed in the Fedora 44 asfats5 container with GCC 16.1.1:

  • ASan: test_tsutil "[AtomicSharedPtr]" passed (11 assertions in 2 test cases) with no sanitizer findings.
  • TSan: test_tsutil "[AtomicSharedPtr]" passed (11 assertions in 2 test cases) with no sanitizer findings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

include/tsutil/AtomicSharedPtr.h:46

  • The PR description mentions removing stale library-version shorthand from the fallback comments, but the earlier comment block still hard-codes libstdc++/libc++ version cutoffs. Consider rewording that top-level comment to describe the feature-based condition instead, so the header stays consistent with the clarified fallback comment below.
// Belt-and-suspenders: on toolchains that take this branch, the free-function
// overloads are normally not marked [[deprecated]], so the suppression below is
// usually a no-op.  It matters only if someone forces the fallback on a modern
// library (e.g. -D__cpp_lib_atomic_shared_ptr=0) or compiles against a library
// that ships the deprecation markers ahead of the specialization.

Comment thread src/tsutil/unit_tests/test_AtomicSharedPtr.cc
AtomicSharedPtr protects shared remap configuration during concurrent
reloads, but it lacked focused regression coverage. An incorrect load
or swap could silently reintroduce a use-after-free race.

This adds Catch2 coverage for load, store, and exchange operations and
for concurrent readers during repeated writer swaps. It also documents
the fallback using the feature-test macro that selects it.
@bneradt
bneradt force-pushed the test-atomic-shared-ptr branch from 0bfbe73 to 3da28be Compare August 18, 2026 23:03
Copilot AI review requested due to automatic review settings August 18, 2026 23:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment on lines +53 to +54
std::atomic<bool> should_start{false};
std::atomic<bool> should_stop{false};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The std::atomic_flag type would be more suitable for these flags and would simplify the waiting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants