From a743915d72957be63ee19afc3a406f3562870f32 Mon Sep 17 00:00:00 2001 From: eidheim Date: Tue, 25 Jun 2019 11:29:36 +0200 Subject: [PATCH] Made use of clang's Thread Safety Analysis --- CMakeLists.txt | 3 ++ client_http.hpp | 30 ++++++------ mutex.hpp | 106 +++++++++++++++++++++++++++++++++++++++++++ server_http.hpp | 51 +++++++++++---------- tests/CMakeLists.txt | 3 ++ 5 files changed, 153 insertions(+), 40 deletions(-) create mode 100644 mutex.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index c40c405..c60b032 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7,6 +7,9 @@ option(BUILD_TESTING "set ON to build library tests" OFF) if(NOT MSVC) add_compile_options(-std=c++11 -Wall -Wextra -Wsign-conversion) + if (CMAKE_CXX_COMPILER_ID MATCHES "Clang") + add_compile_options(-Wthread-safety) + endif() else() add_compile_options(/W1) endif() diff --git a/client_http.hpp b/client_http.hpp index 1bf61c4..128b01f 100644 --- a/client_http.hpp +++ b/client_http.hpp @@ -2,9 +2,9 @@ #define CLIENT_HTTP_HPP #include "asio_compatibility.hpp" +#include "mutex.hpp" #include "utility.hpp" #include -#include #include #include #include @@ -208,12 +208,12 @@ namespace SimpleWeb { }); { - std::lock_guard lock(concurrent_synchronous_requests_mutex); + LockGuard lock(concurrent_synchronous_requests_mutex); ++concurrent_synchronous_requests; } io_service->run(); { - std::lock_guard lock(concurrent_synchronous_requests_mutex); + LockGuard lock(concurrent_synchronous_requests_mutex); --concurrent_synchronous_requests; if(!concurrent_synchronous_requests) restart(*io_service); @@ -238,12 +238,12 @@ namespace SimpleWeb { }); { - std::lock_guard lock(concurrent_synchronous_requests_mutex); + LockGuard lock(concurrent_synchronous_requests_mutex); ++concurrent_synchronous_requests; } io_service->run(); { - std::lock_guard lock(concurrent_synchronous_requests_mutex); + LockGuard lock(concurrent_synchronous_requests_mutex); --concurrent_synchronous_requests; if(!concurrent_synchronous_requests) restart(*io_service); @@ -266,7 +266,7 @@ namespace SimpleWeb { session->callback = [this, session_weak, request_callback](const error_code &ec) { if(auto session = session_weak.lock()) { { - std::lock_guard lock(this->connections_mutex); + LockGuard lock(this->connections_mutex); if(!session->connection->event_stream) session->connection->in_use = false; @@ -341,7 +341,7 @@ namespace SimpleWeb { session->callback = [this, session_weak, request_callback](const error_code &ec) { if(auto session = session_weak.lock()) { { - std::lock_guard lock(this->connections_mutex); + LockGuard lock(this->connections_mutex); if(!session->connection->event_stream) session->connection->in_use = false; @@ -396,7 +396,7 @@ namespace SimpleWeb { /// Close connections void stop() noexcept { - std::lock_guard lock(connections_mutex); + LockGuard lock(connections_mutex); for(auto it = connections.begin(); it != connections.end();) { (*it)->close(); it = connections.erase(it); @@ -417,13 +417,13 @@ namespace SimpleWeb { std::unique_ptr> host_port; - std::unordered_set> connections; - std::mutex connections_mutex; + Mutex connections_mutex; + std::unordered_set> connections GUARDED_BY(connections_mutex); std::shared_ptr handler_runner; - std::size_t concurrent_synchronous_requests = 0; - std::mutex concurrent_synchronous_requests_mutex; + Mutex concurrent_synchronous_requests_mutex; + std::size_t concurrent_synchronous_requests GUARDED_BY(concurrent_synchronous_requests_mutex) = 0; ClientBase(const std::string &host_port, unsigned short default_port) noexcept : default_port(default_port), handler_runner(new ScopeRunner()) { auto parsed_host_port = parse_host_port(host_port, default_port); @@ -433,7 +433,7 @@ namespace SimpleWeb { std::shared_ptr get_connection() noexcept { std::shared_ptr connection; - std::lock_guard lock(connections_mutex); + LockGuard lock(connections_mutex); if(!io_service) { io_service = std::make_shared(); @@ -586,7 +586,7 @@ namespace SimpleWeb { if(!ec) { { - std::lock_guard lock(this->connections_mutex); + LockGuard lock(this->connections_mutex); this->connections.erase(session->connection); } session->callback(ec); @@ -616,7 +616,7 @@ namespace SimpleWeb { } else { if(session->connection->attempt_reconnect && ec != error::operation_aborted) { - std::unique_lock lock(connections_mutex); + LockGuard lock(connections_mutex); auto it = connections.find(session->connection); if(it != connections.end()) { connections.erase(it); diff --git a/mutex.hpp b/mutex.hpp new file mode 100644 index 0000000..755c20c --- /dev/null +++ b/mutex.hpp @@ -0,0 +1,106 @@ +// Based on https://clang.llvm.org/docs/ThreadSafetyAnalysis.html +#ifndef SIMPLE_WEB_MUTEX_HPP +#define SIMPLE_WEB_MUTEX_HPP + +#include + +// Enable thread safety attributes only with clang. +#if defined(__clang__) && (!defined(SWIG)) +#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x)) +#else +#define THREAD_ANNOTATION_ATTRIBUTE__(x) // no-op +#endif + +#define CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(capability(x)) + +#define SCOPED_CAPABILITY \ + THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable) + +#define GUARDED_BY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x)) + +#define PT_GUARDED_BY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_by(x)) + +#define ACQUIRED_BEFORE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquired_before(__VA_ARGS__)) + +#define ACQUIRED_AFTER(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquired_after(__VA_ARGS__)) + +#define REQUIRES(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(requires_capability(__VA_ARGS__)) + +#define REQUIRES_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(requires_shared_capability(__VA_ARGS__)) + +#define ACQUIRE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquire_capability(__VA_ARGS__)) + +#define ACQUIRE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquire_shared_capability(__VA_ARGS__)) + +#define RELEASE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(release_capability(__VA_ARGS__)) + +#define RELEASE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(release_shared_capability(__VA_ARGS__)) + +#define TRY_ACQUIRE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_capability(__VA_ARGS__)) + +#define TRY_ACQUIRE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_shared_capability(__VA_ARGS__)) + +#define EXCLUDES(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__)) + +#define ASSERT_CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(assert_capability(x)) + +#define ASSERT_SHARED_CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(assert_shared_capability(x)) + +#define RETURN_CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x)) + +#define NO_THREAD_SAFETY_ANALYSIS \ + THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis) + +namespace SimpleWeb { + /// Defines an annotated interface for mutexes. + class CAPABILITY("mutex") Mutex { + std::mutex mutex; + + public: + void lock() ACQUIRE() { + mutex.lock(); + } + + void unlock() RELEASE() { + mutex.unlock(); + } + }; + + class SCOPED_CAPABILITY LockGuard { + Mutex &mutex; + bool locked = true; + + public: + LockGuard(Mutex &mutex_) ACQUIRE(mutex_) : mutex(mutex_) { + mutex.lock(); + } + void unlock() RELEASE() { + mutex.unlock(); + locked = false; + } + ~LockGuard() RELEASE() { + if(locked) + mutex.unlock(); + } + }; + +} // namespace SimpleWeb + +#endif // SIMPLE_WEB_MUTEX_HPP diff --git a/server_http.hpp b/server_http.hpp index 85ddf47..c4741fd 100644 --- a/server_http.hpp +++ b/server_http.hpp @@ -2,13 +2,13 @@ #define SERVER_HTTP_HPP #include "asio_compatibility.hpp" +#include "mutex.hpp" #include "utility.hpp" #include #include #include #include #include -#include #include #include #include @@ -45,8 +45,8 @@ namespace SimpleWeb { std::shared_ptr session; long timeout_content; - std::mutex send_queue_mutex; - std::list, std::function>> send_queue; + Mutex send_queue_mutex; + std::list, std::function>> send_queue GUARDED_BY(send_queue_mutex); Response(std::shared_ptr session_, long timeout_content) noexcept : std::ostream(nullptr), session(std::move(session_)), timeout_content(timeout_content) { rdbuf(streambuf.get()); @@ -70,15 +70,14 @@ namespace SimpleWeb { *this << "\r\n"; } - /// send_queue_mutex must be locked here - void send_from_queue() { + void send_from_queue() REQUIRES(send_queue_mutex) { auto self = this->shared_from_this(); asio::async_write(*self->session->connection->socket, *send_queue.begin()->first, [self](const error_code &ec, std::size_t /*bytes_transferred*/) { auto lock = self->session->connection->handler_runner->continue_lock(); if(!lock) return; { - std::unique_lock lock(self->send_queue_mutex); + LockGuard lock(self->send_queue_mutex); if(!ec) { auto it = self->send_queue.begin(); auto callback = std::move(it->second); @@ -133,7 +132,7 @@ namespace SimpleWeb { this->streambuf = std::unique_ptr(new asio::streambuf()); rdbuf(this->streambuf.get()); - std::lock_guard lock(send_queue_mutex); + LockGuard lock(send_queue_mutex); send_queue.emplace_back(streambuf, callback); if(send_queue.size() == 1) send_from_queue(); @@ -451,10 +450,10 @@ namespace SimpleWeb { acceptor->close(ec); { - std::lock_guard lock(*connections_mutex); - for(auto &connection : *connections) + LockGuard lock(connections->mutex); + for(auto &connection : connections->set) connection->close(); - connections->clear(); + connections->set.clear(); } if(internal_io_service) @@ -473,12 +472,15 @@ namespace SimpleWeb { std::unique_ptr acceptor; std::vector threads; - std::shared_ptr> connections; - std::shared_ptr connections_mutex; + struct Connections { + Mutex mutex; + std::unordered_set set GUARDED_BY(mutex); + }; + std::shared_ptr connections; std::shared_ptr handler_runner; - ServerBase(unsigned short port) noexcept : config(port), connections(new std::unordered_set()), connections_mutex(new std::mutex()), handler_runner(new ScopeRunner()) {} + ServerBase(unsigned short port) noexcept : config(port), connections(new Connections()), handler_runner(new ScopeRunner()) {} virtual void after_bind() {} virtual void accept() = 0; @@ -486,19 +488,18 @@ namespace SimpleWeb { template std::shared_ptr create_connection(Args &&... args) noexcept { auto connections = this->connections; - auto connections_mutex = this->connections_mutex; - auto connection = std::shared_ptr(new Connection(handler_runner, std::forward(args)...), [connections, connections_mutex](Connection *connection) { + auto connection = std::shared_ptr(new Connection(handler_runner, std::forward(args)...), [connections](Connection *connection) { { - std::lock_guard lock(*connections_mutex); - auto it = connections->find(connection); - if(it != connections->end()) - connections->erase(it); + LockGuard lock(connections->mutex); + auto it = connections->set.find(connection); + if(it != connections->set.end()) + connections->set.erase(it); } delete connection; }); { - std::lock_guard lock(*connections_mutex); - connections->emplace(connection.get()); + LockGuard lock(connections->mutex); + connections->set.emplace(connection.get()); } return connection; } @@ -684,10 +685,10 @@ namespace SimpleWeb { if(it != session->request->header.end()) { // remove connection from connections { - std::lock_guard lock(*connections_mutex); - auto it = connections->find(session->connection.get()); - if(it != connections->end()) - connections->erase(it); + LockGuard lock(connections->mutex); + auto it = connections->set.find(session->connection.get()); + if(it != connections->set.end()) + connections->set.erase(it); } on_upgrade(session->connection->socket, session->request); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 081fe73..4491826 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,5 +1,8 @@ if(NOT MSVC) add_compile_options(-fno-access-control) + if (CMAKE_CXX_COMPILER_ID MATCHES "Clang") + add_compile_options(-Wno-thread-safety) + endif() add_executable(io_test io_test.cpp) target_link_libraries(io_test simple-web-server)