Skip to content

Commit

Permalink
pw_sync: [[nodiscard]] for try_lock() and similar functions
Browse files Browse the repository at this point in the history
Ignoring the return value of a try_lock() function could be a serious
issue.

Change-Id: Id59215af0333ad3b5a15dd5eb6bc4ead5db8f656
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/229311
Pigweed-Auto-Submit: Wyatt Hepler <[email protected]>
Commit-Queue: Wyatt Hepler <[email protected]>
Reviewed-by: Taylor Cramer <[email protected]>
Reviewed-by: Ewout van Bekkum <[email protected]>
Lint: Lint 🤖 <[email protected]>
  • Loading branch information
255 authored and CQ Bot Account committed Aug 27, 2024
1 parent 8d8bb25 commit 2a34a28
Show file tree
Hide file tree
Showing 10 changed files with 29 additions and 23 deletions.
3 changes: 2 additions & 1 deletion pw_async_basic/dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "pw_async_basic/dispatcher.h"

#include <mutex>
#include <utility>

#include "pw_chrono/system_clock.h"

Expand Down Expand Up @@ -74,7 +75,7 @@ void BasicDispatcher::MaybeSleep() {
}
lock_.unlock();
if (wake_time.has_value()) {
timed_notification_.try_acquire_until(*wake_time);
std::ignore = timed_notification_.try_acquire_until(*wake_time);
} else {
timed_notification_.acquire();
}
Expand Down
4 changes: 3 additions & 1 deletion pw_log_rpc/public/pw_log_rpc/rpc_log_drain_thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include <cstddef>
#include <optional>
#include <utility>

#include "pw_chrono/system_clock.h"
#include "pw_log_rpc/log_service.h"
Expand Down Expand Up @@ -63,7 +64,8 @@ class RpcLogDrainThread : public thread::ThreadCore,
chrono::SystemClock::duration::zero();
while (true) {
if (drains_pending && min_delay.has_value()) {
ready_to_flush_notification_.try_acquire_for(min_delay.value());
std::ignore =
ready_to_flush_notification_.try_acquire_for(min_delay.value());
} else {
ready_to_flush_notification_.acquire();
}
Expand Down
11 changes: 6 additions & 5 deletions pw_sync/public/pw_sync/binary_semaphore.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class BinarySemaphore {
/// @retval true if the internal counter was reset successfully.
///
/// This is thread and IRQ safe.
bool try_acquire() noexcept;
[[nodiscard]] bool try_acquire() noexcept;

/// Tries to decrement the internal counter to 0. Blocks until the specified
/// timeout has elapsed or the counter was decremented to 0, whichever comes
Expand All @@ -80,7 +80,7 @@ class BinarySemaphore {
/// @retval true if the internal counter was decremented successfully.
///
/// This is thread safe, but not IRQ safe.
bool try_acquire_for(chrono::SystemClock::duration timeout);
[[nodiscard]] bool try_acquire_for(chrono::SystemClock::duration timeout);

/// Tries to decrement the internal counter to 0. Blocks until the specified
/// deadline has been reached or the counter was decremented to 0, whichever
Expand All @@ -89,15 +89,16 @@ class BinarySemaphore {
/// @retval true if the internal counter was decremented successfully.
///
/// This is thread safe, but not IRQ safe.
bool try_acquire_until(chrono::SystemClock::time_point deadline);
[[nodiscard]] bool try_acquire_until(
chrono::SystemClock::time_point deadline);

/// @retval backend::kBinarySemaphoreMaxValue the internal counter's maximum
/// possible value.
static constexpr ptrdiff_t max() noexcept {
[[nodiscard]] static constexpr ptrdiff_t max() noexcept {
return backend::kBinarySemaphoreMaxValue;
}

native_handle_type native_handle();
[[nodiscard]] native_handle_type native_handle();

private:
/// This may be a wrapper around a native type with additional members.
Expand Down
9 changes: 5 additions & 4 deletions pw_sync/public/pw_sync/counting_semaphore.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,15 @@ class CountingSemaphore {
/// Returns true if the internal counter was decremented successfully.
///
/// This is IRQ safe.
bool try_acquire() noexcept;
[[nodiscard]] bool try_acquire() noexcept;

/// Tries to decrement the internal counter by 1. Blocks until the specified
/// timeout has elapsed or the counter was decremented by 1, whichever comes
/// first.
///
/// Returns true if the internal counter was decremented successfully.
/// This is thread safe, but not IRQ safe.
bool try_acquire_for(chrono::SystemClock::duration timeout);
[[nodiscard]] bool try_acquire_for(chrono::SystemClock::duration timeout);

/// Tries to decrement the internal counter by 1. Blocks until the specified
/// deadline has been reached or the counter was decremented by 1, whichever
Expand All @@ -89,10 +89,11 @@ class CountingSemaphore {
/// Returns true if the internal counter was decremented successfully.
///
/// This is thread safe, but not IRQ safe.
bool try_acquire_until(chrono::SystemClock::time_point deadline);
[[nodiscard]] bool try_acquire_until(
chrono::SystemClock::time_point deadline);

/// Returns the internal counter's maximum possible value.
static constexpr ptrdiff_t max() noexcept {
[[nodiscard]] static constexpr ptrdiff_t max() noexcept {
return backend::kCountingSemaphoreMaxValue;
}

Expand Down
4 changes: 2 additions & 2 deletions pw_sync/public/pw_sync/interrupt_spin_lock.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,15 @@ class PW_LOCKABLE("pw::sync::InterruptSpinLock") InterruptSpinLock {
/// Returns true if the spinlock was successfully acquired.
///
/// @b Precondition: Recursive locking is undefined behavior.
bool try_lock() PW_EXCLUSIVE_TRYLOCK_FUNCTION(true);
[[nodiscard]] bool try_lock() PW_EXCLUSIVE_TRYLOCK_FUNCTION(true);

/// Unlocks the spinlock. Failures are fatal.
///
/// @b Precondition:
/// The spinlock is held by the caller.
void unlock() PW_UNLOCK_FUNCTION();

native_handle_type native_handle();
[[nodiscard]] native_handle_type native_handle();

private:
/// This may be a wrapper around a native type with additional members.
Expand Down
4 changes: 2 additions & 2 deletions pw_sync/public/pw_sync/mutex.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,15 @@ class PW_LOCKABLE("pw::sync::Mutex") Mutex {
/// @b PRECONDITION:
/// The lock isn't already held by this thread. Recursive locking is
/// undefined behavior.
bool try_lock() PW_EXCLUSIVE_TRYLOCK_FUNCTION(true);
[[nodiscard]] bool try_lock() PW_EXCLUSIVE_TRYLOCK_FUNCTION(true);

/// Unlocks the mutex. Failures are fatal.
///
/// @b PRECONDITION:
/// The mutex is held by this thread.
void unlock() PW_UNLOCK_FUNCTION();

native_handle_type native_handle();
[[nodiscard]] native_handle_type native_handle();

protected:
/// Expose the NativeMutex directly to derived classes (TimedMutex) in case
Expand Down
4 changes: 2 additions & 2 deletions pw_sync/public/pw_sync/recursive_mutex.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,12 @@ class PW_LOCKABLE("pw::sync::RecursiveMutex") RecursiveMutex {

// Attempts to lock the mutex in a non-blocking manner.
// Returns true if the mutex was successfully acquired.
bool try_lock() PW_EXCLUSIVE_TRYLOCK_FUNCTION(true);
[[nodiscard]] bool try_lock() PW_EXCLUSIVE_TRYLOCK_FUNCTION(true);

// Unlocks the mutex. Failures are fatal.
void unlock() PW_UNLOCK_FUNCTION();

native_handle_type native_handle();
[[nodiscard]] native_handle_type native_handle();

private:
// This may be a wrapper around a native type with additional members.
Expand Down
4 changes: 2 additions & 2 deletions pw_sync/public/pw_sync/thread_notification.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class ThreadNotification {
/// was reset successfully.
///
/// @b IMPORTANT: This should only be used by a single consumer thread.
bool try_acquire();
[[nodiscard]] bool try_acquire();

/// Notifies the thread in a saturating manner, setting the notification
/// latch.
Expand All @@ -74,7 +74,7 @@ class ThreadNotification {
/// This is IRQ and thread safe.
void release();

native_handle_type native_handle();
[[nodiscard]] native_handle_type native_handle();

private:
/// This may be a wrapper around a native type with additional members.
Expand Down
4 changes: 2 additions & 2 deletions pw_sync/public/pw_sync/timed_mutex.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class TimedMutex : public Mutex {
/// @b PRECONDITION:
/// The lock isn't already held by this thread. Recursive locking is
/// undefined behavior.
bool try_lock_for(chrono::SystemClock::duration timeout)
[[nodiscard]] bool try_lock_for(chrono::SystemClock::duration timeout)
PW_EXCLUSIVE_TRYLOCK_FUNCTION(true);

/// Tries to lock the mutex. Blocks until specified deadline has been reached
Expand All @@ -66,7 +66,7 @@ class TimedMutex : public Mutex {
/// @b PRECONDITION:
/// The lock isn't already held by this thread. Recursive locking is
/// undefined behavior.
bool try_lock_until(chrono::SystemClock::time_point deadline)
[[nodiscard]] bool try_lock_until(chrono::SystemClock::time_point deadline)
PW_EXCLUSIVE_TRYLOCK_FUNCTION(true);
};

Expand Down
5 changes: 3 additions & 2 deletions pw_sync/public/pw_sync/timed_thread_notification.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class TimedThreadNotification : public ThreadNotification {
/// was reset successfully.
///
/// @b IMPORTANT: This should only be used by a single consumer thread.
bool try_acquire_for(chrono::SystemClock::duration timeout);
[[nodiscard]] bool try_acquire_for(chrono::SystemClock::duration timeout);

/// Blocks until the specified deadline time has been reached the thread has
/// been notified (i.e. notification latch can be cleared because it was set),
Expand All @@ -66,7 +66,8 @@ class TimedThreadNotification : public ThreadNotification {
/// was reset successfully.
///
/// @b IMPORTANT: This should only be used by a single consumer thread.
bool try_acquire_until(chrono::SystemClock::time_point deadline);
[[nodiscard]] bool try_acquire_until(
chrono::SystemClock::time_point deadline);
};

} // namespace pw::sync
Expand Down

0 comments on commit 2a34a28

Please sign in to comment.