This is an automated email from the ASF dual-hosted git repository.
moonchen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 2b6dce0314 Introduce Clang Thread Safety Analysis, and apply it to two
subsystems (#13310)
2b6dce0314 is described below
commit 2b6dce0314f32f95860fcef3acaef8397aeb2cc4
Author: Mo Chen <[email protected]>
AuthorDate: Tue Jun 23 22:11:59 2026 -0500
Introduce Clang Thread Safety Analysis, and apply it to two subsystems
(#13310)
Add TS_* annotation macros (tsutil/ts_thread_safety.h) wrapping Clang's
-Wthread-safety attributes, so a lock's contract -- which mutex guards
which data, which lock a function requires its caller to hold -- can be
expressed in the type system and proved at build time. They expand to
nothing off Clang: no runtime cost, and a no-op for GCC.
Add annotated lock types for the analysis to track: ts::mutex with
ts::lock_guard, and ts::shared_mutex (already ATS's own rwlock) marked
as a capability with ts::write_guard / ts::read_guard. Annotated code
takes its locks through these because the std:: RAII wrappers are too
flexible -- deferred locking, move, adopt/release -- for the analysis to
track, and ATS does not need that flexibility. The guard names mirror
std::lock_guard's rigid acquire-in-constructor / release-in-destructor
RAII rather than implying std::unique_lock's flexibility.
Add the ENABLE_THREAD_SAFETY_ANALYSIS option (Clang-only, on by default
as a warning) and THREAD_SAFETY_ANALYSIS_AS_ERROR, which the CI and
branch presets enable so violations are errors that gate merges while
local and dev builds stay warnings. Install the new headers with the
rest of tsutil, and add a unit test compiled with the analysis enabled
as a worked example. Skip FreeBSD: its libc annotates the pthread
primitives themselves, so -Wthread-safety there flags ATS's existing
hand-rolled mutex wrappers (tscore/ink_mutex.h and others) tree-wide;
bringing FreeBSD into the gate needs those legacy wrappers made
analysis-clean first.
Apply the analysis to two subsystems:
Metrics::Storage: valid(), lookup(IdType) and name() read
_cur_blob/_cur_off/_blobs with no lock held, while create()/createSpan()/
current() access the same fields under the mutex (rename() likewise read
them before locking). A single Storage is shared by all threads, so a
metric registered at runtime -- a plugin TSStatCreate or a config reload
-- advances those fields while live traffic reads them: a data race.
Take the mutex on every access and mark the fields guarded by it; the
reads that were missing a lock take it exclusively, matching the existing
locked paths.
SSLOriginSessionCache: the origin session map and queue are reachable
from every thread; mark them guarded by the cache mutex so the compiler
enforces the locking that was previously only convention. Replace the
hand-rolled lock witness on remove_oldest_session (a std::unique_lock
parameter checked with owns_lock()) with a compile-time TS_REQUIRES
precondition.
---
CMakeLists.txt | 31 +++++
CMakePresets.json | 2 +
include/tsutil/Metrics.h | 18 +--
include/tsutil/TsMutex.h | 100 +++++++++++++++
include/tsutil/TsSharedMutex.h | 54 +++++++--
include/tsutil/ts_thread_safety.h | 97 +++++++++++++++
src/iocore/net/SSLSessionCache.cc | 21 ++--
src/iocore/net/SSLSessionCache.h | 8 +-
src/tsutil/CMakeLists.txt | 15 +++
src/tsutil/Metrics.cc | 16 +--
src/tsutil/unit_tests/test_thread_safety.cc | 182 ++++++++++++++++++++++++++++
11 files changed, 505 insertions(+), 39 deletions(-)
diff --git a/CMakeLists.txt b/CMakeLists.txt
index db0ead0822..d28c3bd289 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -170,6 +170,8 @@ option(EXTERNAL_HWY "Use external highway (default OFF)")
option(LINK_PLUGINS "Link core libraries to plugins (default OFF)")
option(ENABLE_PROBES "Enable ATS SystemTap probes (default OFF)")
option(ENABLE_VERIFY_PLUGINS "Enable plugin verification tests (default ON)"
ON)
+option(ENABLE_THREAD_SAFETY_ANALYSIS "Enable Clang -Wthread-safety analysis
(Clang only, default ON)" ON)
+option(THREAD_SAFETY_ANALYSIS_AS_ERROR "Treat thread-safety findings as
errors; for CI gating (default OFF)")
# Setup user
# NOTE: this is the user trafficserver runs as
@@ -359,6 +361,35 @@ elseif(ENABLE_TSAN)
add_link_options(-g -fsanitize=thread)
endif()
+# Clang Thread Safety Analysis. The TS_* annotations
(tsutil/ts_thread_safety.h)
+# compile to nothing on GCC, and the flag is Clang-only on purpose (GCC does
not
+# know -Wthread-safety and would itself error if passed it), so this whole
block
+# is a no-op for GCC builds.
+#
+# On Clang it is on by default but only a WARNING: -Wno-error=thread-safety
keeps
+# it a warning even in builds that otherwise use -Werror, so an in-progress
+# annotation never blocks a developer's build. CI sets
+# THREAD_SAFETY_ANALYSIS_AS_ERROR=ON to promote findings to errors and gate
+# merges.
+#
+# Skip FreeBSD: its libc annotates the pthread primitives themselves, so
+# -Wthread-safety there flags ATS's existing hand-rolled mutex wrappers
+# (tscore/ink_mutex.h, ink_rwlock, ...) tree-wide, not just newly-annotated
code.
+# Enabling it on FreeBSD needs those legacy wrappers made analysis-clean first.
+# Elsewhere the platform leaves the pthread primitives un-annotated, so only
+# annotated code is analyzed and this stays quiet until a real violation.
+if(ENABLE_THREAD_SAFETY_ANALYSIS
+ AND CMAKE_CXX_COMPILER_ID MATCHES "Clang"
+ AND NOT CMAKE_SYSTEM_NAME STREQUAL "FreeBSD"
+)
+ add_compile_options(-Wthread-safety)
+ if(THREAD_SAFETY_ANALYSIS_AS_ERROR)
+ add_compile_options(-Werror=thread-safety)
+ else()
+ add_compile_options(-Wno-error=thread-safety)
+ endif()
+endif()
+
if(ENABLE_PROBES)
add_compile_options("-DENABLE_SYSTEMTAP_PROBES")
endif()
diff --git a/CMakePresets.json b/CMakePresets.json
index 6e0735c87f..cec863644f 100644
--- a/CMakePresets.json
+++ b/CMakePresets.json
@@ -125,6 +125,8 @@
"cacheVariables": {
"CMAKE_BUILD_TYPE": "Debug",
"CMAKE_COMPILE_WARNING_AS_ERROR": "ON",
+ "ENABLE_THREAD_SAFETY_ANALYSIS": "ON",
+ "THREAD_SAFETY_ANALYSIS_AS_ERROR": "ON",
"ENABLE_CCACHE": "ON",
"BUILD_EXPERIMENTAL_PLUGINS": "ON",
"ENABLE_WASM_WAMR": "OFF",
diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h
index 630f079b4d..4f47fd5485 100644
--- a/include/tsutil/Metrics.h
+++ b/include/tsutil/Metrics.h
@@ -38,6 +38,7 @@
#include "swoc/MemSpan.h"
#include "tsutil/Assert.h"
+#include "tsutil/TsMutex.h"
namespace ts
{
@@ -304,17 +305,17 @@ private:
class Storage
{
- BlobStorage _blobs;
- uint16_t _cur_blob = 0;
- uint16_t _cur_off = 0;
- LookupTable _lookups;
- mutable std::mutex _mutex;
+ BlobStorage _blobs TS_GUARDED_BY(_mutex);
+ uint16_t _cur_blob TS_GUARDED_BY(_mutex) = 0;
+ uint16_t _cur_off TS_GUARDED_BY(_mutex) = 0;
+ LookupTable _lookups TS_GUARDED_BY(_mutex);
+ mutable ts::mutex _mutex;
public:
Storage(const Storage &) = delete;
Storage &operator=(const Storage &) = delete;
- Storage()
+ Storage() TS_NO_THREAD_SAFETY_ANALYSIS // single-threaded construction;
not yet shared
{
_blobs[0] = std::make_unique<NamesAndAtomics>();
release_assert(_blobs[0]);
@@ -325,7 +326,7 @@ private:
~Storage() {}
IdType create(const std::string_view name, const MetricType type
= MetricType::COUNTER);
- void addBlob();
+ void addBlob() TS_REQUIRES(_mutex);
IdType lookup(const std::string_view name) const;
AtomicType *lookup(const std::string_view name, IdType *out_id,
MetricType *out_type = nullptr) const;
AtomicType *lookup(Metrics::IdType id, std::string_view *out_name =
nullptr, MetricType *out_type = nullptr) const;
@@ -337,7 +338,7 @@ private:
std::pair<int16_t, int16_t>
current() const
{
- std::lock_guard lock(_mutex);
+ ts::lock_guard lock(_mutex);
return {_cur_blob, _cur_off};
}
@@ -346,6 +347,7 @@ private:
{
auto [blob, entry] = _splitID(id);
+ ts::lock_guard lock(_mutex);
return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob ==
_cur_blob && entry <= _cur_off)));
}
};
diff --git a/include/tsutil/TsMutex.h b/include/tsutil/TsMutex.h
new file mode 100644
index 0000000000..6bc193dc4e
--- /dev/null
+++ b/include/tsutil/TsMutex.h
@@ -0,0 +1,100 @@
+/** @file
+
+ A std::mutex annotated for Clang Thread Safety Analysis, with a matching
+ scoped lock guard.
+
+ These are the plain-mutex counterparts to ts::shared_mutex and its
+ reader/writer guards (TsSharedMutex.h): use ts::mutex with ts::lock_guard
+ wherever you would otherwise use std::mutex with std::lock_guard, but want
the
+ data it protects checked by -Wthread-safety. The runtime behavior is exactly
+ that of std::mutex; the annotations are compile-time only (see
+ tsutil/ts_thread_safety.h).
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#pragma once
+
+#include <mutex>
+
+#include "tsutil/ts_thread_safety.h"
+
+namespace ts
+{
+// A std::mutex marked as a Clang thread-safety capability, so data guarded by
it
+// can be checked by -Wthread-safety. Same interface and runtime behavior as
+// std::mutex.
+//
+class TS_CAPABILITY("mutex") mutex
+{
+public:
+ mutex() = default;
+ mutex(mutex const &) = delete;
+ mutex &operator=(mutex const &) = delete;
+
+ // The lock/unlock bodies are the trusted implementation of this capability:
+ // exempt them from analysis so only the capability contract on each
signature
+ // is checked. The actual data-race checking happens at the call sites.
+ void
+ lock() TS_ACQUIRE() TS_NO_THREAD_SAFETY_ANALYSIS
+ {
+ _m.lock();
+ }
+ bool
+ try_lock() TS_TRY_ACQUIRE(true) TS_NO_THREAD_SAFETY_ANALYSIS
+ {
+ return _m.try_lock();
+ }
+ void
+ unlock() TS_RELEASE() TS_NO_THREAD_SAFETY_ANALYSIS
+ {
+ _m.unlock();
+ }
+
+ using native_handle_type = std::mutex::native_handle_type;
+
+ native_handle_type
+ native_handle()
+ {
+ return _m.native_handle();
+ }
+
+private:
+ std::mutex _m;
+};
+
+// RAII guard for ts::mutex that carries Clang thread-safety capability state.
+// Prefer over std::lock_guard / std::unique_lock in code annotated for
+// -Wthread-safety (see tsutil/ts_thread_safety.h for why the std wrappers are
+// not tracked).
+//
+class TS_SCOPED_CAPABILITY lock_guard
+{
+public:
+ explicit lock_guard(mutex &m) TS_ACQUIRE(m) : _m(m) { _m.lock(); }
+ ~lock_guard() TS_RELEASE() { _m.unlock(); }
+
+ lock_guard(lock_guard const &) = delete;
+ lock_guard &operator=(lock_guard const &) = delete;
+
+private:
+ mutex &_m;
+};
+
+} // end namespace ts
diff --git a/include/tsutil/TsSharedMutex.h b/include/tsutil/TsSharedMutex.h
index ccd025c98a..bf09885040 100644
--- a/include/tsutil/TsSharedMutex.h
+++ b/include/tsutil/TsSharedMutex.h
@@ -27,6 +27,7 @@
#include <pthread.h>
#include "tsutil/Strerror.h"
#include "tsutil/Assert.h"
+#include "tsutil/ts_thread_safety.h"
#ifdef X
#error "X preprocessor symbol defined"
@@ -48,7 +49,7 @@ namespace ts
{
// A class with the same interface as std::shared_mutex, but which is not
prone to writer starvation.
//
-class shared_mutex
+class TS_CAPABILITY("shared_mutex") shared_mutex
{
public:
shared_mutex() {}
@@ -58,8 +59,13 @@ public:
shared_mutex(shared_mutex const &) = delete;
shared_mutex &operator=(shared_mutex const &) = delete;
+ // The lock/unlock methods are the trusted implementation of this capability:
+ // their bodies drive the raw pthread_rwlock_t, which some libc headers (e.g.
+ // FreeBSD's <pthread.h>) annotate as a capability in its own right. Exempt
the
+ // bodies from analysis so only the capability contract on each signature is
+ // checked; the actual data-race checking happens at the call sites.
void
- lock()
+ lock() TS_ACQUIRE() TS_NO_THREAD_SAFETY_ANALYSIS
{
int error = pthread_rwlock_wrlock(&_lock);
if (error != 0) {
@@ -69,7 +75,7 @@ public:
}
bool
- try_lock()
+ try_lock() TS_TRY_ACQUIRE(true) TS_NO_THREAD_SAFETY_ANALYSIS
{
int error = pthread_rwlock_trywrlock(&_lock);
if (EBUSY == error) {
@@ -84,7 +90,7 @@ public:
}
void
- unlock()
+ unlock() TS_RELEASE() TS_NO_THREAD_SAFETY_ANALYSIS
{
X(debug_assert(_exclusive);)
X(_exclusive = false;)
@@ -93,7 +99,7 @@ public:
}
void
- lock_shared()
+ lock_shared() TS_ACQUIRE_SHARED() TS_NO_THREAD_SAFETY_ANALYSIS
{
int error = pthread_rwlock_rdlock(&_lock);
if (error != 0) {
@@ -105,7 +111,7 @@ public:
}
bool
- try_lock_shared()
+ try_lock_shared() TS_TRY_ACQUIRE_SHARED(true) TS_NO_THREAD_SAFETY_ANALYSIS
{
int error = pthread_rwlock_tryrdlock(&_lock);
if (EBUSY == error) {
@@ -121,7 +127,7 @@ public:
}
void
- unlock_shared()
+ unlock_shared() TS_RELEASE_SHARED() TS_NO_THREAD_SAFETY_ANALYSIS
{
X(debug_assert(_shared > 0);)
X(--_shared;)
@@ -148,7 +154,7 @@ public:
private:
void
- _unlock()
+ _unlock() TS_NO_THREAD_SAFETY_ANALYSIS
{
int error = pthread_rwlock_unlock(&_lock);
if (error != 0) {
@@ -178,6 +184,38 @@ private:
X(std::atomic<int> _shared{0};)
};
+// RAII guards for ts::shared_mutex that carry Clang thread-safety capability
+// state. Prefer these over std::unique_lock / std::shared_lock in code
annotated
+// for -Wthread-safety: the analysis does not reliably track the std wrappers
+// (see tsutil/ts_thread_safety.h).
+//
+class TS_SCOPED_CAPABILITY write_guard
+{
+public:
+ explicit write_guard(shared_mutex &m) TS_ACQUIRE(m) : _m(m) { _m.lock(); }
+ ~write_guard() TS_RELEASE() { _m.unlock(); }
+
+ write_guard(write_guard const &) = delete;
+ write_guard &operator=(write_guard const &) = delete;
+
+private:
+ shared_mutex &_m;
+};
+
+class TS_SCOPED_CAPABILITY read_guard
+{
+public:
+ explicit read_guard(shared_mutex &m) TS_ACQUIRE_SHARED(m) : _m(m) {
_m.lock_shared(); }
+ // A scoped-capability destructor uses the plain release form even for a
shared acquire.
+ ~read_guard() TS_RELEASE() { _m.unlock_shared(); }
+
+ read_guard(read_guard const &) = delete;
+ read_guard &operator=(read_guard const &) = delete;
+
+private:
+ shared_mutex &_m;
+};
+
} // end namespace ts
#undef X
diff --git a/include/tsutil/ts_thread_safety.h
b/include/tsutil/ts_thread_safety.h
new file mode 100644
index 0000000000..36e3b36e4e
--- /dev/null
+++ b/include/tsutil/ts_thread_safety.h
@@ -0,0 +1,97 @@
+/** @file
+
+ Clang Thread Safety Analysis annotation macros.
+
+ These wrap Clang's @c -Wthread-safety attributes so a lock's contract can be
+ expressed in the type system: which mutex guards which data, and which lock a
+ function requires its caller to hold. The compiler then proves, at build
time,
+ that guarded data is only touched while the right lock is held.
+
+ The annotations are compile-time only. Under Clang they expand to
+ @c __attribute__((...)) consumed by the analysis; under every other compiler
+ they expand to nothing. They generate no code and have zero runtime cost.
+
+ Important note: Clang's analysis only tracks lock state through types marked
as
+ capabilities (the mutex) and scoped capabilities (the RAII guard). The @c
std::
+ lock wrappers are deliberately flexible -- deferred locking, adopt/release,
and
+ movability let the held state escape a single scope -- which makes it
impossible
+ to track statically, and ATS does not need that flexibility. Annotated code
+ therefore takes its locks through ATS-owned annotated types -- @c ts::mutex
with
+ @c ts::lock_guard, or @c ts::shared_mutex with @c ts::write_guard /
+ @c ts::read_guard -- whose simple acquire-in-constructor /
+ release-in-destructor contract the analysis can follow.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#pragma once
+
+#if defined(__clang__)
+#define TS_THREAD_ANNOTATION(x) __attribute__((x))
+#else
+#define TS_THREAD_ANNOTATION(x)
+#endif
+
+/// Mark a class as a capability (a lock-like object that can be "held").
+#define TS_CAPABILITY(name) TS_THREAD_ANNOTATION(capability(name))
+
+/// Mark an RAII type whose lifetime holds a capability (acquire in ctor,
+/// release in dtor), e.g. a scoped lock guard.
+#define TS_SCOPED_CAPABILITY TS_THREAD_ANNOTATION(scoped_lockable)
+
+/// The annotated data member may only be accessed while @a x is held.
+#define TS_GUARDED_BY(x) TS_THREAD_ANNOTATION(guarded_by(x))
+
+/// The data pointed to by the annotated pointer may only be accessed while
+/// @a x is held.
+#define TS_PT_GUARDED_BY(x) TS_THREAD_ANNOTATION(pt_guarded_by(x))
+
+/// The function acquires the listed capabilities (exclusively / shared).
+#define TS_ACQUIRE(...)
TS_THREAD_ANNOTATION(acquire_capability(__VA_ARGS__))
+#define TS_ACQUIRE_SHARED(...)
TS_THREAD_ANNOTATION(acquire_shared_capability(__VA_ARGS__))
+
+/// The function releases the listed capabilities. Use the plain form (not the
+/// shared form) on a scoped-capability destructor even for a shared guard.
+#define TS_RELEASE(...)
TS_THREAD_ANNOTATION(release_capability(__VA_ARGS__))
+#define TS_RELEASE_SHARED(...)
TS_THREAD_ANNOTATION(release_shared_capability(__VA_ARGS__))
+
+/// The function conditionally acquires a capability, holding it only on the
+/// branch where it returns @a success_value.
+#define TS_TRY_ACQUIRE(...)
TS_THREAD_ANNOTATION(try_acquire_capability(__VA_ARGS__))
+#define TS_TRY_ACQUIRE_SHARED(...)
TS_THREAD_ANNOTATION(try_acquire_shared_capability(__VA_ARGS__))
+
+/// The caller must already hold the listed capabilities (exclusively /
shared).
+#define TS_REQUIRES(...)
TS_THREAD_ANNOTATION(requires_capability(__VA_ARGS__))
+#define TS_REQUIRES_SHARED(...)
TS_THREAD_ANNOTATION(requires_shared_capability(__VA_ARGS__))
+
+/// The caller must NOT hold the listed capabilities (prevents self-deadlock).
+#define TS_EXCLUDES(...) TS_THREAD_ANNOTATION(locks_excluded(__VA_ARGS__))
+
+/// Declare that a getter returns a reference to the named capability.
+#define TS_RETURN_CAPABILITY(x) TS_THREAD_ANNOTATION(lock_returned(x))
+
+/// Tell the analysis a capability is held here without acquiring it (runtime
+/// assertion form).
+#define TS_ASSERT_CAPABILITY(x)
TS_THREAD_ANNOTATION(assert_capability(x))
+#define TS_ASSERT_SHARED_CAPABILITY(x)
TS_THREAD_ANNOTATION(assert_shared_capability(x))
+
+/// Disable the analysis for a single function. Use sparingly, for code the
+/// analysis cannot model (recursive acquire, hand-off across threads,
+/// single-threaded destructors).
+#define TS_NO_THREAD_SAFETY_ANALYSIS
TS_THREAD_ANNOTATION(no_thread_safety_analysis)
diff --git a/src/iocore/net/SSLSessionCache.cc
b/src/iocore/net/SSLSessionCache.cc
index 2a68f26868..43fb6fc530 100644
--- a/src/iocore/net/SSLSessionCache.cc
+++ b/src/iocore/net/SSLSessionCache.cc
@@ -43,7 +43,7 @@ SSLSessDeleter(SSL_SESSION *_p)
SSLOriginSessionCache::SSLOriginSessionCache() {}
-SSLOriginSessionCache::~SSLOriginSessionCache()
+SSLOriginSessionCache::~SSLOriginSessionCache() TS_NO_THREAD_SAFETY_ANALYSIS
// single-threaded teardown
{
while (auto *node = orig_sess_que.pop()) {
delete node;
@@ -78,8 +78,8 @@ SSLOriginSessionCache::insert_session(const std::string
&lookup_key, SSL_SESSION
new SSLOriginSession(lookup_key, curve, group_name,
std::shared_ptr<SSL_SESSION>{sess_ptr, SSLSessDeleter}));
auto new_node = ssl_orig_session.release();
- std::unique_lock lock(mutex);
- auto entry = orig_sess_map.find(lookup_key);
+ ts::write_guard lock(mutex);
+ auto entry = orig_sess_map.find(lookup_key);
if (entry != orig_sess_map.end()) {
auto node = entry->second;
Dbg(dbg_ctl_ssl_origin_session_cache, "found duplicate key: %s, replacing
%p with %p", lookup_key.c_str(),
@@ -89,7 +89,7 @@ SSLOriginSessionCache::insert_session(const std::string
&lookup_key, SSL_SESSION
delete node;
} else if (orig_sess_map.size() >=
SSLConfigParams::origin_session_cache_size) {
Dbg(dbg_ctl_ssl_origin_session_cache, "origin session cache full, removing
oldest session");
- remove_oldest_session(lock);
+ remove_oldest_session();
}
orig_sess_que.enqueue(new_node);
@@ -101,8 +101,8 @@ SSLOriginSessionCache::get_session(const std::string
&lookup_key, ssl_curve_id *
{
Dbg(dbg_ctl_ssl_origin_session_cache, "get session: %s", lookup_key.c_str());
- std::shared_lock lock(mutex);
- auto entry = orig_sess_map.find(lookup_key);
+ ts::read_guard lock(mutex);
+ auto entry = orig_sess_map.find(lookup_key);
if (entry == orig_sess_map.end()) {
return nullptr;
}
@@ -117,11 +117,8 @@ SSLOriginSessionCache::get_session(const std::string
&lookup_key, ssl_curve_id *
}
void
-SSLOriginSessionCache::remove_oldest_session(const
std::unique_lock<ts::shared_mutex> &lock)
+SSLOriginSessionCache::remove_oldest_session()
{
- // Caller must hold the bucket shared_mutex with unique_lock.
- ink_release_assert(lock.owns_lock());
-
while (orig_sess_que.head && orig_sess_que.size >=
static_cast<int>(SSLConfigParams::origin_session_cache_size)) {
auto node = orig_sess_que.pop();
Dbg(dbg_ctl_ssl_origin_session_cache, "remove oldest session: %s, session
ptr: %p", node->key.c_str(), node->shared_sess.get());
@@ -134,8 +131,8 @@ void
SSLOriginSessionCache::remove_session(const std::string &lookup_key)
{
// We can't bail on contention here because this session MUST be removed.
- std::unique_lock lock(mutex);
- auto entry = orig_sess_map.find(lookup_key);
+ ts::write_guard lock(mutex);
+ auto entry = orig_sess_map.find(lookup_key);
if (entry != orig_sess_map.end()) {
auto node = entry->second;
Dbg(dbg_ctl_ssl_origin_session_cache, "remove session: %s, session ptr:
%p", lookup_key.c_str(), node->shared_sess.get());
diff --git a/src/iocore/net/SSLSessionCache.h b/src/iocore/net/SSLSessionCache.h
index ff78e8f0e1..6e0b9bdc66 100644
--- a/src/iocore/net/SSLSessionCache.h
+++ b/src/iocore/net/SSLSessionCache.h
@@ -79,9 +79,9 @@ public:
void remove_session(const std::string &lookup_key);
private:
- void remove_oldest_session(const std::unique_lock<ts::shared_mutex> &lock);
+ void remove_oldest_session() TS_REQUIRES(mutex);
- mutable ts::shared_mutex mutex;
- CountQueue<SSLOriginSession> orig_sess_que;
- std::map<std::string, SSLOriginSession *> orig_sess_map;
+ mutable ts::shared_mutex mutex;
+ CountQueue<SSLOriginSession> orig_sess_que TS_GUARDED_BY(mutex);
+ std::map<std::string, SSLOriginSession *> orig_sess_map TS_GUARDED_BY(mutex);
};
diff --git a/src/tsutil/CMakeLists.txt b/src/tsutil/CMakeLists.txt
index 49be0fb553..7d0e9a651c 100644
--- a/src/tsutil/CMakeLists.txt
+++ b/src/tsutil/CMakeLists.txt
@@ -32,7 +32,9 @@ set(TSUTIL_PUBLIC_HEADERS
${PROJECT_SOURCE_DIR}/include/tsutil/Strerror.h
${PROJECT_SOURCE_DIR}/include/tsutil/StringConvert.h
${PROJECT_SOURCE_DIR}/include/tsutil/Regex.h
+ ${PROJECT_SOURCE_DIR}/include/tsutil/TsMutex.h
${PROJECT_SOURCE_DIR}/include/tsutil/TsSharedMutex.h
+ ${PROJECT_SOURCE_DIR}/include/tsutil/ts_thread_safety.h
${PROJECT_SOURCE_DIR}/include/tsutil/YamlCfg.h
${PROJECT_SOURCE_DIR}/include/tsutil/ts_ip.h
${PROJECT_SOURCE_DIR}/include/tsutil/ts_meta.h
@@ -80,9 +82,22 @@ if(BUILD_TESTING)
unit_tests/test_StringConvert.cc
unit_tests/test_Regex.cc
unit_tests/test_ts_meta.cc
+ unit_tests/test_thread_safety.cc
unit_tests/test_time_parser.cc
)
+ # The thread-safety test doubles as a compile-time check: build it with the
+ # analysis enabled and treat any finding as an error so a broken annotation
+ # chain fails the build. Scoped to the same lanes as the global analysis
block
+ # in the top-level CMakeLists.txt (Clang, excluding FreeBSD); a no-op for
GCC,
+ # where the macros expand to nothing.
+ if(NOT CMAKE_SYSTEM_NAME STREQUAL "FreeBSD")
+ set_source_files_properties(
+ unit_tests/test_thread_safety.cc
+ PROPERTIES COMPILE_OPTIONS
"$<$<CXX_COMPILER_ID:Clang,AppleClang>:-Wthread-safety;-Werror=thread-safety>"
+ )
+ endif()
+
target_link_libraries(test_tsutil PRIVATE tsutil Catch2::Catch2WithMain)
add_catch2_test(NAME test_tsutil COMMAND $<TARGET_FILE:test_tsutil>)
diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc
index ae96c1dbba..92bcb3bf6b 100644
--- a/src/tsutil/Metrics.cc
+++ b/src/tsutil/Metrics.cc
@@ -57,8 +57,8 @@ Metrics::Storage::addBlob() // The mutex must be held before
calling this!
Metrics::IdType
Metrics::Storage::create(std::string_view name, const MetricType type)
{
- std::lock_guard lock(_mutex);
- auto it = _lookups.find(name);
+ ts::lock_guard lock(_mutex);
+ auto it = _lookups.find(name);
if (it != _lookups.end()) {
return it->second;
@@ -81,8 +81,8 @@ Metrics::Storage::create(std::string_view name, const
MetricType type)
Metrics::IdType
Metrics::Storage::lookup(const std::string_view name) const
{
- std::lock_guard lock(_mutex);
- auto it = _lookups.find(name);
+ ts::lock_guard lock(_mutex);
+ auto it = _lookups.find(name);
if (it != _lookups.end()) {
return it->second;
@@ -94,6 +94,7 @@ Metrics::Storage::lookup(const std::string_view name) const
Metrics::AtomicType *
Metrics::Storage::lookup(Metrics::IdType id, std::string_view *out_name,
Metrics::MetricType *out_type) const
{
+ ts::lock_guard lock(_mutex);
auto [blob_ix, offset] = _splitID(id);
Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
@@ -140,6 +141,7 @@ Metrics::Storage::lookup(const std::string_view name,
Metrics::IdType *out_id, M
std::string_view
Metrics::Storage::name(Metrics::IdType id) const
{
+ ts::lock_guard lock(_mutex);
auto [blob_ix, offset] = _splitID(id);
Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
@@ -164,7 +166,7 @@ Metrics::SpanType
Metrics::Storage::createSpan(size_t size, Metrics::MetricType type,
Metrics::IdType *id)
{
release_assert(size <= MAX_SIZE);
- std::lock_guard lock(_mutex);
+ ts::lock_guard lock(_mutex);
if (_cur_off + size > MAX_SIZE) {
addBlob();
@@ -187,6 +189,7 @@ Metrics::Storage::createSpan(size_t size,
Metrics::MetricType type, Metrics::IdT
bool
Metrics::Storage::rename(Metrics::IdType id, std::string_view name)
{
+ ts::lock_guard lock(_mutex);
auto [blob_ix, offset] = _splitID(id);
Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
@@ -195,8 +198,7 @@ Metrics::Storage::rename(Metrics::IdType id,
std::string_view name)
return false;
}
- std::string &cur = std::get<0>(std::get<0>(*blob)[offset]);
- std::lock_guard lock(_mutex);
+ std::string &cur = std::get<0>(std::get<0>(*blob)[offset]);
if (cur.length() > 0) {
_lookups.erase(cur);
diff --git a/src/tsutil/unit_tests/test_thread_safety.cc
b/src/tsutil/unit_tests/test_thread_safety.cc
new file mode 100644
index 0000000000..1325534977
--- /dev/null
+++ b/src/tsutil/unit_tests/test_thread_safety.cc
@@ -0,0 +1,182 @@
+/** @file
+
+ End-to-end test of the Clang thread-safety annotation chain.
+
+ This translation unit is compiled with -Wthread-safety -Werror=thread-safety
+ under Clang (see CMakeLists.txt), so it doubles as a compile-time assertion:
+ the annotated example below must analyze cleanly, and any access to the
+ protected data outside a held guard would fail the build.
+
+ The chain demonstrated here is:
+ * ts::mutex / ts::shared_mutex are capabilities (TsMutex.h,
+ TsSharedMutex.h),
+ * ts::lock_guard / ts::write_guard /
+ ts::read_guard supply the held capability for
+ the duration of a scope (TsMutex.h,
+ TsSharedMutex.h),
+ * the data members below are annotated as protected by that capability.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "tsutil/TsMutex.h"
+#include "tsutil/TsSharedMutex.h"
+
+#include <catch2/catch_test_macros.hpp>
+
+#include <map>
+#include <string>
+#include <thread>
+#include <vector>
+
+namespace
+{
+class ProtectedTable
+{
+public:
+ void
+ put(const std::string &key, int value)
+ {
+ ts::write_guard lock(_mutex);
+ _map[key] = value; // write under exclusive lock
+ bump_locked(); // calls a TS_REQUIRES helper while the lock is held
+ }
+
+ int
+ get(const std::string &key)
+ {
+ ts::read_guard lock(_mutex);
+ auto it = _map.find(key); // read under shared lock
+ return it == _map.end() ? -1 : it->second;
+ }
+
+ int
+ writes() const
+ {
+ ts::read_guard lock(_mutex);
+ return _writes;
+ }
+
+private:
+ // The compiler proves the caller holds _mutex; no runtime witness needed.
+ void
+ bump_locked() TS_REQUIRES(_mutex)
+ {
+ ++_writes;
+ }
+
+ mutable ts::shared_mutex _mutex;
+ std::map<std::string, int> _map TS_GUARDED_BY(_mutex);
+ int _writes TS_GUARDED_BY(_mutex) = 0;
+};
+
+// The plain-mutex counterpart: ts::mutex guarded data taken through
+// ts::lock_guard.
+class ProtectedCounter
+{
+public:
+ void
+ add(int n)
+ {
+ ts::lock_guard lock(_mutex);
+ _sum += n; // write under exclusive lock
+ bump_locked(); // calls a TS_REQUIRES helper while the lock is held
+ }
+
+ int
+ sum() const
+ {
+ ts::lock_guard lock(_mutex);
+ return _sum;
+ }
+
+ int
+ adds() const
+ {
+ ts::lock_guard lock(_mutex);
+ return _adds;
+ }
+
+private:
+ void
+ bump_locked() TS_REQUIRES(_mutex)
+ {
+ ++_adds;
+ }
+
+ mutable ts::mutex _mutex;
+ int _sum TS_GUARDED_BY(_mutex) = 0;
+ int _adds TS_GUARDED_BY(_mutex) = 0;
+};
+
+// Counterexample (kept as documentation): adding an unguarded accessor such as
+// int peek() const { return _writes; }
+// to ProtectedTable makes this file fail to compile under
-Werror=thread-safety,
+// with: "reading variable '_writes' requires holding shared_mutex '_mutex'".
+// That compile-time failure is the whole point.
+} // namespace
+
+TEST_CASE("ts::shared_mutex annotated guards provide mutual exclusion",
"[thread_safety]")
+{
+ ProtectedTable table;
+
+ constexpr int n_threads = 8;
+ constexpr int per_thread = 1000;
+
+ std::vector<std::thread> threads;
+ threads.reserve(n_threads);
+ for (int t = 0; t < n_threads; ++t) {
+ threads.emplace_back([&table, t]() {
+ const std::string key = "k" + std::to_string(t);
+ for (int i = 0; i < per_thread; ++i) {
+ table.put(key, i);
+ (void)table.get(key);
+ }
+ });
+ }
+ for (auto &th : threads) {
+ th.join();
+ }
+
+ REQUIRE(table.writes() == n_threads * per_thread);
+}
+
+TEST_CASE("ts::mutex annotated guard provides mutual exclusion",
"[thread_safety]")
+{
+ ProtectedCounter counter;
+
+ constexpr int n_threads = 8;
+ constexpr int per_thread = 1000;
+
+ std::vector<std::thread> threads;
+ threads.reserve(n_threads);
+ for (int t = 0; t < n_threads; ++t) {
+ threads.emplace_back([&counter]() {
+ for (int i = 0; i < per_thread; ++i) {
+ counter.add(1);
+ }
+ });
+ }
+ for (auto &th : threads) {
+ th.join();
+ }
+
+ REQUIRE(counter.sum() == n_threads * per_thread);
+ REQUIRE(counter.adds() == n_threads * per_thread);
+}
