Skip to content

Commit

Permalink
Fix potential deadlock.
Browse files Browse the repository at this point in the history
This could occur if there were multiple consumers waiting on an empty queue, at least one of which is trying to call `pop_one`, and the producer adds a single element. If, after that operation, the producer adds at least one more element before the one awakened consumer can call `pop_one`, then the producer will not alert any other consumers that there is more data in the queue and they will not have a chance to be reawakened until the queue is empty again or their thread is cancelled.

There are two possible fixes to this issue.

The first option is to always notify all consumers if the queue was empty and it's possible one of the consumers is asking for a single element, rather than the current behavior of the notification count depending on whether we are adding one or several.

The second option is to unconditionally notify one consumer, regardless of whether the queue was empty.

This commit goes with option 1: it means we call `notify_all` when a `notify_one` would have sufficed in some cases, but it means that multiple consecutive calls to add data do not notify anyone. This seems like the right trade-off, but some use cases may prefer the other option.
  • Loading branch information
davidstone committed Jan 4, 2024
1 parent 68e8825 commit ae0d530
Showing 1 changed file with 6 additions and 14 deletions.
20 changes: 6 additions & 14 deletions source/queue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,25 +35,19 @@ struct basic_queue_impl {
// This will also optimize memory usage, as the underlying container can
// reserve all the space it needs.
auto append(containers::range auto && input) -> void {
constexpr auto adding_several = std::true_type{};
generic_add(
adding_several,
[&]{ containers::append(m_container, OPERATORS_FORWARD(input)); }
);
}

auto non_blocking_append(containers::range auto && input) -> bool {
constexpr auto adding_several = std::true_type{};
return generic_non_blocking_add(
adding_several,
[&]{ containers::append(m_container, OPERATORS_FORWARD(input)); }
);
}

auto emplace(auto && ... args) -> void {
constexpr auto adding_several = std::false_type{};
generic_add(
adding_several,
[&]{ containers::emplace_back(m_container, OPERATORS_FORWARD(args)...); }
);
}
Expand All @@ -65,9 +59,7 @@ struct basic_queue_impl {
}

auto non_blocking_emplace(auto && ... args) -> bool {
constexpr auto adding_several = std::false_type{};
return generic_non_blocking_add(
adding_several,
[&]{ containers::emplace_back(m_container, OPERATORS_FORWARD(args)...); }
);
}
Expand Down Expand Up @@ -252,20 +244,20 @@ struct basic_queue_impl {
}


auto generic_add(auto const adding_several, auto && add) -> void {
generic_add_impl(adding_several, lock_type(m_mutex), add);
auto generic_add(auto && add) -> void {
generic_add_impl(lock_type(m_mutex), add);
}

auto generic_non_blocking_add(auto const adding_several, auto && add) -> bool {
auto generic_non_blocking_add(auto && add) -> bool {
auto lock = lock_type(m_mutex, std::try_to_lock);
if (!lock.owns_lock()) {
return false;
}
generic_add_impl(adding_several, std::move(lock), add);
generic_add_impl(std::move(lock), add);
return true;
}

auto generic_add_impl(auto const adding_several, lock_type lock, auto && add) -> void {
auto generic_add_impl(lock_type lock, auto && add) -> void {
derived().handle_add(m_container, lock);
auto const was_empty = containers::is_empty(m_container);
add();
Expand Down Expand Up @@ -304,7 +296,7 @@ struct basic_queue_impl {
// A, B, 1, 2: Same as A, 1, 2, B. 2 never waits because 1 does not
// find an empty container.
if (was_empty) {
if constexpr (pop_frontable<Container> && adding_several) {
if constexpr (pop_frontable<Container>) {
m_notify_addition.notify_all();
} else {
m_notify_addition.notify_one();
Expand Down

0 comments on commit ae0d530

Please sign in to comment.