Skip to content

Conversation

brodeuralexis
Copy link
Contributor

In line with fine::Mutex and fine::SharedMutex, this commit adds a wrapper, fine::ConditionVariable for ErlNifCond with an API similar if not identifical to std::condition_variable.

In line with `fine::Mutex` and `fine::SharedMutex`, this commit adds a
wrapper, `fine::ConditionVariable` for `ErlNifCond` with an API similar
if not identifical to `std::condition_variable`.
Comment on lines +275 to +280
[[deprecated(
"usage of `void fine::ConditionVariable::wait(std::unique_lock<Mutex> "
"&, Predicate)` is preferred")]]
void wait(std::unique_lock<Mutex> &lock) noexcept {
enif_cond_wait(m_handle.get(), *lock.mutex());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we even expose this function?
If we remove it, I am afraid a user will use enif_cond_wait directly without understanding that enif_cond_wait can return earlier than expected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecation is not really accurate for this case, so I would either skip deprecation or remove the function.

The same concern goes for std::condition_variable::wait, right? So if we want to maintain API compatibility, we should probably include it. That said, I don't have strong opinion, so pick whichever approach you consider better :)


std::thread thread4([&] {
notified = true;
cond.notify_one();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically thread4 could run first and then none of the other threads needs to wait, because the condition is already true.

We can make the test more robust, by having a num_threads_counter and each thread after obtaining the lock does num_threads_counter++. Then thread4 can wait for num_threads_counter == 4, using another condition variable. wdyt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants