From 70e8eaa4d84f46aae928b47c2c2ec916a250fdca Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Wed, 12 Jul 2023 19:48:18 +0000 Subject: [PATCH 1/2] Implement thread-safe logging. This commit introduces thread-safe logging to rcutils. In particular, what it does is to add in a read-write lock around accesses to the g_rcutils_logging_severities_map in logging.c. This enables two things: 1. Thread-safe writes to the hash map so that get_logger_level and set_logger_level can be called simultaneously on two different threads. 2. The ability to cache lookups during get_logger_effective_level, so we have to do less parsing on every log call. This introduces the concept of locking into rcutils, which we haven't had before. However, this particular use seems to be justified, since the logging subsystem can be called outside of the client libraries that could possibly do this locking at a higher level (think about the rmw implementations, for instance). Note that this introduces a new pthread dependency within rcutils that we've not had before. Signed-off-by: Chris Lalancette --- CMakeLists.txt | 1 + src/logging.c | 76 ++++++++++++--- src/rwlock.c | 250 +++++++++++++++++++++++++++++++++++++++++++++++++ src/rwlock.h | 70 ++++++++++++++ 4 files changed, 383 insertions(+), 14 deletions(-) create mode 100644 src/rwlock.c create mode 100644 src/rwlock.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 35d4df0c..38ae04f9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -63,6 +63,7 @@ set(rcutils_sources src/process.c src/qsort.c src/repl_str.c + src/rwlock.c src/sha256.c src/shared_library.c src/snprintf.c diff --git a/src/logging.c b/src/logging.c index 186ba24c..90b4359f 100644 --- a/src/logging.c +++ b/src/logging.c @@ -49,6 +49,8 @@ #include "rcutils/time.h" #include "rcutils/types/hash_map.h" +#include "./rwlock.h" + #define RCUTILS_LOGGING_SEPARATOR_CHAR '.' @@ -90,6 +92,7 @@ static const char * g_rcutils_logging_default_output_format = static rcutils_allocator_t g_rcutils_logging_allocator; static rcutils_logging_output_handler_t g_rcutils_logging_output_handler = NULL; +static rcutils_rwlock_t g_rcutils_logging_map_lock; static rcutils_hash_map_t g_rcutils_logging_severities_map; // If this is false, attempts to use the severities map will be skipped. @@ -645,6 +648,17 @@ rcutils_ret_t rcutils_logging_initialize_with_allocator(rcutils_allocator_t allo return RCUTILS_RET_ERROR; } + g_rcutils_logging_map_lock = rcutils_get_zero_initialized_rwlock(); + + if (rcutils_rwlock_init(&g_rcutils_logging_map_lock, allocator) != RCUTILS_RET_OK) { + RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( + "Failed to initialize rwlock for logger severities [%s].", rcutils_get_error_string().str); + if (rcutils_hash_map_fini(&g_rcutils_logging_severities_map) != RCUTILS_RET_OK) { + RCUTILS_SAFE_FWRITE_TO_STDERR("Additionally, failed to free memory when cleaning up\n"); + } + return RCUTILS_RET_ERROR; + } + parse_and_create_handlers_list(); g_rcutils_logging_severities_map_valid = true; @@ -665,6 +679,7 @@ rcutils_ret_t rcutils_logging_shutdown(void) // Iterate over the map, getting every key so we can free it char * key = NULL; int level; + rcutils_rwlock_write_lock(&g_rcutils_logging_map_lock); rcutils_ret_t hash_map_ret = rcutils_hash_map_get_next_key_and_data( &g_rcutils_logging_severities_map, NULL, &key, &level); while (RCUTILS_RET_OK == hash_map_ret) { @@ -688,6 +703,9 @@ rcutils_ret_t rcutils_logging_shutdown(void) rcutils_get_error_string().str); ret = RCUTILS_RET_LOGGING_SEVERITY_MAP_INVALID; } + rcutils_rwlock_write_unlock(&g_rcutils_logging_map_lock); + rcutils_rwlock_fini(&g_rcutils_logging_map_lock); + g_rcutils_logging_severities_map_valid = false; } g_num_log_msg_handlers = 0; @@ -775,6 +793,18 @@ static rcutils_ret_t add_key_to_hash_map(const char * name, int level, bool set_ // Check if key already exists, to avoid extra memory allocation // If the key already exists, then rcutils_hash_map_set will not maintain the key we give it, // so we do not need to copy the name + if (set_by_user) { + rcutils_rwlock_write_lock(&g_rcutils_logging_map_lock); + } else { + // In the specific case where this is not being set by the user, we know + // that this is a cache set. Just try to get the lock here; if we fail, + // the worst case is that we don't cache this in the map and we'll try + // again next time. + rcutils_ret_t lockret = rcutils_rwlock_write_trylock(&g_rcutils_logging_map_lock); + if (lockret != RCUTILS_RET_OK) { + return RCUTILS_RET_OK; + } + } bool already_exists = rcutils_hash_map_key_exists(&g_rcutils_logging_severities_map, ©_name); if (!already_exists) { @@ -783,6 +813,7 @@ static rcutils_ret_t add_key_to_hash_map(const char * name, int level, bool set_ if (copy_name == NULL) { // Don't report an error to the error handling machinery; some uses of this function are for // caching so this is not necessarily fatal. + rcutils_rwlock_write_unlock(&g_rcutils_logging_map_lock); return RCUTILS_RET_ERROR; } } @@ -800,16 +831,21 @@ static rcutils_ret_t add_key_to_hash_map(const char * name, int level, bool set_ RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( "Error setting severity level for logger named '%s': %s", name, rcutils_get_error_string().str); + rcutils_rwlock_write_unlock(&g_rcutils_logging_map_lock); return RCUTILS_RET_ERROR; } + rcutils_rwlock_write_unlock(&g_rcutils_logging_map_lock); + return RCUTILS_RET_OK; } static rcutils_ret_t get_severity_level(const char * name, int * severity) { + rcutils_rwlock_read_lock(&g_rcutils_logging_map_lock); rcutils_ret_t ret = rcutils_hash_map_get(&g_rcutils_logging_severities_map, &name, severity); + rcutils_rwlock_read_unlock(&g_rcutils_logging_map_lock); if (ret != RCUTILS_RET_OK) { // One possible response is RCUTILS_RET_NOT_FOUND, but the higher layers may be OK with that. return ret; @@ -845,7 +881,9 @@ int rcutils_logging_get_logger_leveln(const char * name, size_t name_length) } int severity; + rcutils_rwlock_read_lock(&g_rcutils_logging_map_lock); rcutils_ret_t ret = get_severity_level(short_name, &severity); + rcutils_rwlock_read_unlock(&g_rcutils_logging_map_lock); g_rcutils_logging_allocator.deallocate(short_name, g_rcutils_logging_allocator.state); if (ret != RCUTILS_RET_OK) { // The error message was already set by get_severity_level @@ -863,8 +901,10 @@ int rcutils_logging_get_logger_effective_level(const char * name) } size_t hash_map_size; + rcutils_rwlock_read_lock(&g_rcutils_logging_map_lock); rcutils_ret_t hash_map_ret = rcutils_hash_map_get_size( &g_rcutils_logging_severities_map, &hash_map_size); + rcutils_rwlock_read_unlock(&g_rcutils_logging_map_lock); if (hash_map_ret != RCUTILS_RET_OK) { RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( "Error getting severity level for logger named '%s': %s", @@ -943,20 +983,18 @@ int rcutils_logging_get_logger_effective_level(const char * name) severity = g_rcutils_logging_default_logger_level; } - // TODO(wjwwood): restore or replace this optimization when thread-safety is addressed - // see: https://github.com/ros2/rcutils/pull/393 - // // If the calculated severity is anything but UNSET, we place it into the hashmap for speedier - // // lookup next time. If the severity is UNSET, we don't bother because we are going to have to - // // walk the hierarchy next time anyway. - // if (severity != RCUTILS_LOG_SEVERITY_UNSET) { - // ret = add_key_to_hash_map(name, severity, false); - // if (ret != RCUTILS_RET_OK) { - // // Continue on if we failed to add the key to the hashmap. - // // This will affect performance but is not fatal. - // RCUTILS_SAFE_FWRITE_TO_STDERR( - // "Failed to cache severity; this is not fatal but will impact performance\n"); - // } - // } + // If the calculated severity is anything but UNSET, we place it into the hashmap for speedier + // lookup next time. If the severity is UNSET, we don't bother because we are going to have to + // walk the hierarchy next time anyway. + if (severity != RCUTILS_LOG_SEVERITY_UNSET) { + ret = add_key_to_hash_map(name, severity, false); + if (ret != RCUTILS_RET_OK) { + // Continue on if we failed to add the key to the hashmap. + // This will affect performance but is not fatal. + RCUTILS_SAFE_FWRITE_TO_STDERR( + "Failed to cache severity; this is not fatal but will impact performance\n"); + } + } return severity; } @@ -990,6 +1028,13 @@ rcutils_ret_t rcutils_logging_set_logger_level(const char * name, int level) size_t name_length = strlen(name); + // TODO(clalancette): We could *almost* make this a read lock, except for the fact that the loop + // below might unset keys. If we find that this write lock is a bottleneck, we could split + // that unsetting of keys into a separate function which acquires the write lock itself. + // However, don't forget that when unlocking as read and relocking as write, you have to + // double-check that the thing you were operating on still exists (since it may have been + // changed in the meantime). + rcutils_rwlock_write_lock(&g_rcutils_logging_map_lock); if (rcutils_hash_map_key_exists(&g_rcutils_logging_severities_map, &name)) { // Iterate over the entire contents of the severities map, looking for entries that start with // this key name. For any ones that match, check whether the user explicitly set them. If the @@ -1029,6 +1074,7 @@ rcutils_ret_t rcutils_logging_set_logger_level(const char * name, int level) RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( "Error accessing hash map when setting logger level for '%s': %s", name, rcutils_get_error_string().str); + rcutils_rwlock_write_unlock(&g_rcutils_logging_map_lock); return hash_map_ret; } @@ -1042,12 +1088,14 @@ rcutils_ret_t rcutils_logging_set_logger_level(const char * name, int level) RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( "Error clearing old severity level for logger named '%s': %s", name, rcutils_get_error_string().str); + rcutils_rwlock_write_unlock(&g_rcutils_logging_map_lock); return unset_ret; } g_rcutils_logging_allocator.deallocate(previous_key, g_rcutils_logging_allocator.state); } } } + rcutils_rwlock_write_unlock(&g_rcutils_logging_map_lock); rcutils_ret_t add_key_ret = add_key_to_hash_map(name, level, true); if (add_key_ret != RCUTILS_RET_OK) { diff --git a/src/rwlock.c b/src/rwlock.c new file mode 100644 index 00000000..0970adcc --- /dev/null +++ b/src/rwlock.c @@ -0,0 +1,250 @@ +// Copyright 2023 Open Source Robotics Foundation, Inc. +// +// Licensed 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 "./rwlock.h" // NOLINT + +#include "rcutils/allocator.h" +#include "rcutils/error_handling.h" +#include "rcutils/types/rcutils_ret.h" + +rcutils_rwlock_t +rcutils_get_zero_initialized_rwlock(void) +{ + static rcutils_rwlock_t zero_initialized_rwlock; + zero_initialized_rwlock.impl = NULL; + return zero_initialized_rwlock; +} + +#ifndef _WIN32 + +#include +#include + +typedef struct rcutils_rwlock_impl_s +{ + pthread_rwlock_t lock; + rcutils_allocator_t allocator; +} rcutils_rwlock_impl_t; + +rcutils_ret_t +rcutils_rwlock_init(rcutils_rwlock_t * lock, rcutils_allocator_t allocator) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + if (lock->impl != NULL) { + RCUTILS_SET_ERROR_MSG("rwlock already initialized"); + return RCUTILS_RET_ERROR; + } + RCUTILS_CHECK_ALLOCATOR_WITH_MSG( + &allocator, "invalid allocator", return RCUTILS_RET_INVALID_ARGUMENT); + + lock->impl = allocator.allocate(sizeof(rcutils_rwlock_impl_t), allocator.state); + if (NULL == lock->impl) { + RCUTILS_SET_ERROR_MSG("failed to allocate memory for string map impl struct"); + return RCUTILS_RET_BAD_ALLOC; + } + + lock->impl->allocator = allocator; + + return pthread_rwlock_init(&lock->impl->lock, NULL) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_read_lock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + return pthread_rwlock_rdlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_read_unlock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + return pthread_rwlock_unlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_write_trylock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + return pthread_rwlock_trywrlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_write_lock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + return pthread_rwlock_wrlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_write_unlock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + return pthread_rwlock_unlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_fini(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + if (NULL == lock->impl) { + return RCUTILS_RET_OK; + } + + int retval = pthread_rwlock_destroy(&lock->impl->lock); + + rcutils_allocator_t allocator = lock->impl->allocator; + + allocator.deallocate(lock->impl, allocator.state); + lock->impl = NULL; + + return retval == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +#else + +// When building with MSVC 19.28.29333.0 on Windows 10 (as of 2020-11-11), +// there appears to be a problem with winbase.h (which is included by +// Windows.h). In particular, warnings of the form: +// +// warning C5105: macro expansion producing 'defined' has undefined behavior +// +// See https://developercommunity.visualstudio.com/content/problem/695656/wdk-and-sdk-are-not-compatible-with-experimentalpr.html +// for more information. For now disable that warning when including windows.h +#pragma warning(push) +#pragma warning(disable : 5105) +#include +#pragma warning(pop) + +typedef struct rcutils_rwlock_impl_s +{ + SRWLOCK lock; + rcutils_allocator_t allocator; +} rcutils_rwlock_impl_t; + +rcutils_ret_t +rcutils_rwlock_init(rcutils_rwlock_t * lock, rcutils_allocator_t allocator) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + if (lock->impl != NULL) { + RCUTILS_SET_ERROR_MSG("rwlock already initialized"); + return RCUTILS_RET_ERROR; + } + + RCUTILS_CHECK_ALLOCATOR_WITH_MSG( + &allocator, "invalid allocator", return RCUTILS_RET_INVALID_ARGUMENT); + + lock->impl = allocator.allocate(sizeof(rcutils_rwlock_impl_t), allocator.state); + if (NULL == lock->impl) { + RCUTILS_SET_ERROR_MSG("failed to allocate memory for string map impl struct"); + return RCUTILS_RET_BAD_ALLOC; + } + + lock->impl->allocator = allocator; + + InitializeSRWLock(&lock->impl->lock); + + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_read_lock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + AcquireSRWLockShared(&lock->impl->lock); + + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_read_unlock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + ReleaseSRWLockShared(&lock->impl->lock); + + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_write_trylock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + return TryAcquireSRWLockExclusive(&lock->impl->lock) ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_write_lock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + AcquireSRWLockExclusive(&lock->impl->lock); + + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_write_unlock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + ReleaseSRWLockExclusive(&lock->impl->lock); + + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_fini(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + if (NULL == lock->impl) { + return RCUTILS_RET_OK; + } + + rcutils_allocator_t allocator = lock->impl->allocator; + + allocator.deallocate(lock->impl, allocator.state); + lock->impl = NULL; + + return RCUTILS_RET_OK; +} + +#endif diff --git a/src/rwlock.h b/src/rwlock.h new file mode 100644 index 00000000..ae7bced1 --- /dev/null +++ b/src/rwlock.h @@ -0,0 +1,70 @@ +// Copyright 2023 Open Source Robotics Foundation, Inc. +// +// Licensed 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. + +#ifndef RWLOCK_H_ +#define RWLOCK_H_ + +#ifdef __cplusplus +extern "C" +{ +#endif + +#include "rcutils/allocator.h" +#include "rcutils/types/rcutils_ret.h" +#include "rcutils/visibility_control.h" + +struct rcutils_rwlock_impl_s; + +typedef struct rwlock_s +{ + struct rcutils_rwlock_impl_s * impl; +} rcutils_rwlock_t; + +RCUTILS_PUBLIC +rcutils_rwlock_t +rcutils_get_zero_initialized_rwlock(void); + +RCUTILS_PUBLIC +rcutils_ret_t +rcutils_rwlock_init(rcutils_rwlock_t * lock, rcutils_allocator_t allocator); + +RCUTILS_PUBLIC +rcutils_ret_t +rcutils_rwlock_read_lock(rcutils_rwlock_t * lock); + +RCUTILS_PUBLIC +rcutils_ret_t +rcutils_rwlock_read_unlock(rcutils_rwlock_t * lock); + +RCUTILS_PUBLIC +rcutils_ret_t +rcutils_rwlock_write_lock(rcutils_rwlock_t * lock); + +RCUTILS_PUBLIC +rcutils_ret_t +rcutils_rwlock_write_trylock(rcutils_rwlock_t * lock); + +RCUTILS_PUBLIC +rcutils_ret_t +rcutils_rwlock_write_unlock(rcutils_rwlock_t * lock); + +RCUTILS_PUBLIC +rcutils_ret_t +rcutils_rwlock_fini(rcutils_rwlock_t * lock); + +#ifdef __cplusplus +} +#endif + +#endif // RWLOCK_H_ From ab44fc87ca89ad3477d51f2c85d99422a73010a9 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Wed, 13 Sep 2023 20:35:08 +0000 Subject: [PATCH 2/2] Split up compilation of rwlock to separate files. It is much easier to follow this way. Signed-off-by: Chris Lalancette --- CMakeLists.txt | 16 +++- src/logging.c | 6 ++ src/rwlock_pthread.c | 126 +++++++++++++++++++++++++++++ src/rwlock_stub.c | 74 +++++++++++++++++ src/{rwlock.c => rwlock_win32.c} | 133 ++++--------------------------- 5 files changed, 235 insertions(+), 120 deletions(-) create mode 100644 src/rwlock_pthread.c create mode 100644 src/rwlock_stub.c rename src/{rwlock.c => rwlock_win32.c} (58%) diff --git a/CMakeLists.txt b/CMakeLists.txt index 38ae04f9..4d81b562 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -42,10 +42,24 @@ if(NOT WIN32) add_compile_options(-Wall -Wextra -Wconversion -Wno-sign-conversion -Wpedantic) endif() +option(FORCE_STUB_RWLOCK "Force the use of a 'stub' rwlock implementation, which makes logging slower and inherently racy." OFF) + if(WIN32) set(time_impl_c src/time_win32.c) + set(rwlock_impl_c src/rwlock_win32.c) else() set(time_impl_c src/time_unix.c) + find_package(Threads QUIET) + if(Threads_FOUND) + set(rwlock_impl_c src/rwlock_pthread.c) + else() + if(FORCE_STUB_RWLOCK) + set(rwlock_impl_c src/rwlock_stub.c) + add_definitions(-DRWLOCK_STUB) + else() + message(FATAL "No valid rwlock implementation found!") + endif() + endif() endif() set(rcutils_sources @@ -63,7 +77,7 @@ set(rcutils_sources src/process.c src/qsort.c src/repl_str.c - src/rwlock.c + ${rwlock_impl_c} src/sha256.c src/shared_library.c src/snprintf.c diff --git a/src/logging.c b/src/logging.c index 90b4359f..7a183bcd 100644 --- a/src/logging.c +++ b/src/logging.c @@ -983,6 +983,11 @@ int rcutils_logging_get_logger_effective_level(const char * name) severity = g_rcutils_logging_default_logger_level; } +#if !defined(RWLOCK_STUB) + // If the RWLOCK implementation is a stub, this will definitely cause corruption, even between get + // calls, so we skip it (the consequence is that log level lookups are slower). With a stub + // RWLOCK there is still a race between get and set, but that is less common. + // If the calculated severity is anything but UNSET, we place it into the hashmap for speedier // lookup next time. If the severity is UNSET, we don't bother because we are going to have to // walk the hierarchy next time anyway. @@ -995,6 +1000,7 @@ int rcutils_logging_get_logger_effective_level(const char * name) "Failed to cache severity; this is not fatal but will impact performance\n"); } } +#endif return severity; } diff --git a/src/rwlock_pthread.c b/src/rwlock_pthread.c new file mode 100644 index 00000000..dd35ff99 --- /dev/null +++ b/src/rwlock_pthread.c @@ -0,0 +1,126 @@ +// Copyright 2023 Open Source Robotics Foundation, Inc. +// +// Licensed 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 +#include + +#include "./rwlock.h" // NOLINT + +#include "rcutils/allocator.h" +#include "rcutils/error_handling.h" +#include "rcutils/types/rcutils_ret.h" + +typedef struct rcutils_rwlock_impl_s +{ + pthread_rwlock_t lock; + rcutils_allocator_t allocator; +} rcutils_rwlock_impl_t; + +rcutils_rwlock_t +rcutils_get_zero_initialized_rwlock(void) +{ + static rcutils_rwlock_t zero_initialized_rwlock; + zero_initialized_rwlock.impl = NULL; + return zero_initialized_rwlock; +} + +rcutils_ret_t +rcutils_rwlock_init(rcutils_rwlock_t * lock, rcutils_allocator_t allocator) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + if (lock->impl != NULL) { + RCUTILS_SET_ERROR_MSG("rwlock already initialized"); + return RCUTILS_RET_ERROR; + } + RCUTILS_CHECK_ALLOCATOR_WITH_MSG( + &allocator, "invalid allocator", return RCUTILS_RET_INVALID_ARGUMENT); + + lock->impl = allocator.allocate(sizeof(rcutils_rwlock_impl_t), allocator.state); + if (NULL == lock->impl) { + RCUTILS_SET_ERROR_MSG("failed to allocate memory for string map impl struct"); + return RCUTILS_RET_BAD_ALLOC; + } + + lock->impl->allocator = allocator; + + return pthread_rwlock_init(&lock->impl->lock, NULL) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_read_lock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + return pthread_rwlock_rdlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_read_unlock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + return pthread_rwlock_unlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_write_trylock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + return pthread_rwlock_trywrlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_write_lock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + return pthread_rwlock_wrlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_write_unlock(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + RCUTILS_CHECK_FOR_NULL_WITH_MSG( + lock->impl, "invalid lock", return RCUTILS_RET_ERROR); + + return pthread_rwlock_unlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} + +rcutils_ret_t +rcutils_rwlock_fini(rcutils_rwlock_t * lock) +{ + RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); + if (NULL == lock->impl) { + return RCUTILS_RET_OK; + } + + int retval = pthread_rwlock_destroy(&lock->impl->lock); + + rcutils_allocator_t allocator = lock->impl->allocator; + + allocator.deallocate(lock->impl, allocator.state); + lock->impl = NULL; + + return retval == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; +} diff --git a/src/rwlock_stub.c b/src/rwlock_stub.c new file mode 100644 index 00000000..9a5ef44c --- /dev/null +++ b/src/rwlock_stub.c @@ -0,0 +1,74 @@ +// Copyright 2023 Open Source Robotics Foundation, Inc. +// +// Licensed 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 "./rwlock.h" // NOLINT + +#include "rcutils/allocator.h" +#include "rcutils/types/rcutils_ret.h" + +rcutils_rwlock_t +rcutils_get_zero_initialized_rwlock(void) +{ + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_init(rcutils_rwlock_t * lock, rcutils_allocator_t allocator) +{ + (void)lock; + (void)allocator; + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_read_lock(rcutils_rwlock_t * lock) +{ + (void)lock; + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_read_unlock(rcutils_rwlock_t * lock) +{ + (void)lock; + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_write_lock(rcutils_rwlock_t * lock) +{ + (void)lock; + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_write_trylock(rcutils_rwlock_t * lock) +{ + (void)lock; + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_write_unlock(rcutils_rwlock_t * lock) +{ + (void)lock; + return RCUTILS_RET_OK; +} + +rcutils_ret_t +rcutils_rwlock_fini(rcutils_rwlock_t * lock) +{ + (void)lock; + return RCUTILS_RET_OK; +} diff --git a/src/rwlock.c b/src/rwlock_win32.c similarity index 58% rename from src/rwlock.c rename to src/rwlock_win32.c index 0970adcc..4f95a1b6 100644 --- a/src/rwlock.c +++ b/src/rwlock_win32.c @@ -12,123 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "./rwlock.h" // NOLINT - -#include "rcutils/allocator.h" -#include "rcutils/error_handling.h" -#include "rcutils/types/rcutils_ret.h" - -rcutils_rwlock_t -rcutils_get_zero_initialized_rwlock(void) -{ - static rcutils_rwlock_t zero_initialized_rwlock; - zero_initialized_rwlock.impl = NULL; - return zero_initialized_rwlock; -} - -#ifndef _WIN32 - -#include -#include - -typedef struct rcutils_rwlock_impl_s -{ - pthread_rwlock_t lock; - rcutils_allocator_t allocator; -} rcutils_rwlock_impl_t; - -rcutils_ret_t -rcutils_rwlock_init(rcutils_rwlock_t * lock, rcutils_allocator_t allocator) -{ - RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); - if (lock->impl != NULL) { - RCUTILS_SET_ERROR_MSG("rwlock already initialized"); - return RCUTILS_RET_ERROR; - } - RCUTILS_CHECK_ALLOCATOR_WITH_MSG( - &allocator, "invalid allocator", return RCUTILS_RET_INVALID_ARGUMENT); - - lock->impl = allocator.allocate(sizeof(rcutils_rwlock_impl_t), allocator.state); - if (NULL == lock->impl) { - RCUTILS_SET_ERROR_MSG("failed to allocate memory for string map impl struct"); - return RCUTILS_RET_BAD_ALLOC; - } - - lock->impl->allocator = allocator; - - return pthread_rwlock_init(&lock->impl->lock, NULL) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; -} - -rcutils_ret_t -rcutils_rwlock_read_lock(rcutils_rwlock_t * lock) -{ - RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); - RCUTILS_CHECK_FOR_NULL_WITH_MSG( - lock->impl, "invalid lock", return RCUTILS_RET_ERROR); - - return pthread_rwlock_rdlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; -} - -rcutils_ret_t -rcutils_rwlock_read_unlock(rcutils_rwlock_t * lock) -{ - RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); - RCUTILS_CHECK_FOR_NULL_WITH_MSG( - lock->impl, "invalid lock", return RCUTILS_RET_ERROR); - - return pthread_rwlock_unlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; -} - -rcutils_ret_t -rcutils_rwlock_write_trylock(rcutils_rwlock_t * lock) -{ - RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); - RCUTILS_CHECK_FOR_NULL_WITH_MSG( - lock->impl, "invalid lock", return RCUTILS_RET_ERROR); - - return pthread_rwlock_trywrlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; -} - -rcutils_ret_t -rcutils_rwlock_write_lock(rcutils_rwlock_t * lock) -{ - RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); - RCUTILS_CHECK_FOR_NULL_WITH_MSG( - lock->impl, "invalid lock", return RCUTILS_RET_ERROR); - - return pthread_rwlock_wrlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; -} - -rcutils_ret_t -rcutils_rwlock_write_unlock(rcutils_rwlock_t * lock) -{ - RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); - RCUTILS_CHECK_FOR_NULL_WITH_MSG( - lock->impl, "invalid lock", return RCUTILS_RET_ERROR); - - return pthread_rwlock_unlock(&lock->impl->lock) == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; -} - -rcutils_ret_t -rcutils_rwlock_fini(rcutils_rwlock_t * lock) -{ - RCUTILS_CHECK_ARGUMENT_FOR_NULL(lock, RCUTILS_RET_INVALID_ARGUMENT); - if (NULL == lock->impl) { - return RCUTILS_RET_OK; - } - - int retval = pthread_rwlock_destroy(&lock->impl->lock); - - rcutils_allocator_t allocator = lock->impl->allocator; - - allocator.deallocate(lock->impl, allocator.state); - lock->impl = NULL; - - return retval == 0 ? RCUTILS_RET_OK : RCUTILS_RET_ERROR; -} - -#else - // When building with MSVC 19.28.29333.0 on Windows 10 (as of 2020-11-11), // there appears to be a problem with winbase.h (which is included by // Windows.h). In particular, warnings of the form: @@ -142,12 +25,26 @@ rcutils_rwlock_fini(rcutils_rwlock_t * lock) #include #pragma warning(pop) +#include "./rwlock.h" // NOLINT + +#include "rcutils/allocator.h" +#include "rcutils/error_handling.h" +#include "rcutils/types/rcutils_ret.h" + typedef struct rcutils_rwlock_impl_s { SRWLOCK lock; rcutils_allocator_t allocator; } rcutils_rwlock_impl_t; +rcutils_rwlock_t +rcutils_get_zero_initialized_rwlock(void) +{ + static rcutils_rwlock_t zero_initialized_rwlock; + zero_initialized_rwlock.impl = NULL; + return zero_initialized_rwlock; +} + rcutils_ret_t rcutils_rwlock_init(rcutils_rwlock_t * lock, rcutils_allocator_t allocator) { @@ -246,5 +143,3 @@ rcutils_rwlock_fini(rcutils_rwlock_t * lock) return RCUTILS_RET_OK; } - -#endif