Add coro::condition_variable

[dobord]

Added coro::condition_variable to work together with coro::mutex for scenarios where std::mutex and std::condition_variable need to be replaced

[jbaldwin]

Hello @dobord ,

This is an awesome addition! Thank you for opening a PR.

Can you please add a test/test_condition_variable.cpp with some tests to this PR please? I think it would also be a good idea to add it as an entry in the readme so it is documented with an example like the other coro objects. The README.md file is generated from the .githooks/readme-template.md file, you can enable the custom githooks for this project via the CMake option LIBCORO_RUN_GITCONFIG=ON one time which will then run the README.md generation on git precommit hook after that is completed. The precommit hook also runs clang-format across modified files as well.

I also wonder if you could implement this without the coro::io_scheduler dependency and instead use a modified coro::event that has a notify_all() and notify_one() modification? The modified coro::event::notify_all() would behave just like coro::event::set(true) and awake all awaiters to check the condition variable's predicate(), where notify_one() would awake the first waiter in the linked list of coro::event::m_state. I think this would be beneficial since it would decouple coro::condition_variable from requiring the networking feature flag.

[dobord]

Hello @jbaldwin!
I'm going to add test/test condition_variable.cpp and example to this PR.

Regarding the dependency on coro::io_scheduler: the implementation of coro::condition_variable :: wait_for and wait_until requires coro::io_scheduler, which is able to suspend the execution of a coroutine in a threadpool for a specified time and schedule its launch in a threadpool later.
Without these functions, of course, it would be possible to implement the feature using coro::event. This would not require the networking feature flag. But such a cut-down version of coro::condition_variable would look of little use in those scenarios where a full-fledged replacement for std::condition_variable is needed.

[jbaldwin]

Yeah excellent points, I was only considering the base wait() without the time functionality, so my bad, the event loop like functionality on the coro::io_scheduler is definitely required for this.

[jbaldwin]

Hello @dobord , I'm trying to add a coro::when_any #298. I wonder if this could be used to return the first result of the notify() or the timeout? I noticed you are trying to add a lot of logic around having either the notify or timeout complete and tracking those looks quite difficult.

If the timeout fires the notify task will need to be cancelled somehow, and vice versa (although the timeout task would eventually trigger...).

[dobord]

Hello @jbaldwin, Super! Thanks a lot for the feature coro::when_any #298!
I'll try to simplify the code with it.

While writing tests for coro::condition_variable I found problems that occur in multi-threaded/multi-coroutine scenarios of using coro::condition_variable. Also, spending a file descriptor for each instance of coro::condition_variable seems wasteful. I will make a refactoring that will use the new feature with cancellation of scheduled coro::task by timeout and cancellation token

[dobord]

Hello, @jbaldwin !
Can you help my to fix issues in CI pipeline? I don't know why pipeline ci-fedora, ci-opensuse-15-g++(10,20,ON,ON) are failed?
CI Tests here dobord#4

[jbaldwin]

Hmm, this is a very strange linker error, I'm trying to figure out whats going on but it seems like gcc <= 13 is not including the when_any template functions in the output for some reason.

[dobord]

Hello, @jbaldwin!
I put some tests in test_condition_variable under conditional compilation. This is a workaround to solve the build issue.

[jbaldwin]

Cool, I think that makes sense since the templates are now 'used' so they are not being optimized out.

[jbaldwin]

oops I didn't mean to update, meant to approve and run workflow... you'll have a conflict or need to force push if there are any more changes, my bad, giving it another review now

[jbaldwin]

I had a quick look over the new coro::condition_variable but I need some more time (ran out this morning but I will get to it!).

I really like the idea behind the default executor as well, its a nice addition.

Do you have a need to support emscripten for the example? It seems to complicate it a lot with the custom stop token, just curious.

[jbaldwin]

I think libuv calls this default_loop() for a similar concept implementation, so this could be possibly named default_executor()?

I think its interesting trying to abstract away which executor it is, but I wonder if it would be better to simply have:

1. static coro::default::io_executor() -> coro::io_scheduler&
2. static coro::default::executor() -> coro::thread_pool&

And the user can choose which one to use, I would think they should know if they are compiling with or without the networking support? It should also reduce a lot of the duplicate code from the facade.

[jbaldwin]

Perhaps these should be returning std::shared_ptr<T> since most functions/objects in the library takes those as arguments.

[dobord]

I remade default_executor, now it has 4 global functions in the namespace coro::default_executor.

[dobord]

Hello, @jbaldwin!
I agree, the name default_executor suits this class even better.

It seems impossible to make static coro::default::io_executor and static coro::default::executor(). How to provide user with possibility to set io_scheduler::options and thread_pool::options for creation of these static objects in this case?

In general, I would really like to spread the use of default_executor throughout the library to maximize performance improvements when resuming coroutines.

There is another idea, of course it goes far beyond the scope of this PR#297, I would like to make another coro::scheduler based on coro::thread_pool, which would support the entire interface of coro::io_scheduler except for working with epoll (network, file descriptors, etc.). It would also be cool to make a concept for this scheduler. And after that implement in coro::default_executor the choice between coro::scheduler or coro::io_scheduler as the main scheduler for the application.

[jbaldwin]

How would you feel about splitting out the facade/default_executor to another PR? It feels like its own feature. I might give a stab at the options that you talk about, but I'm also wondering if the defaults for them are good enough?

Regarding the coro::scheduler that works with a backing coro::thread_pool that already exists, since it has two strategies from execution_strategy_t process_tasks_on_thread_pool and process_tasks_inline for how you want it to behave. Perhaps the docs are not clear on this? Maybe I should rename or make it more obvious? Or maybe you mean something else?

[dobord]

I think it's a great idea to split facade/default_executor into another PR.
Should I remove the coro::default_executor code in this PR? If so, what should I put as the default argument here? explicit strategy_based_on_io_scheduler( std::shared_ptr<io_scheduler> io_scheduler = coro::default_executor::instance()->get_io_scheduler());

[dobord]

Regarding the coro::scheduler I meant something else. If the project is built without LIBCORO_FEATURE_NETWORKING, in this case it is currently impossible to use coro::io_scheduler to run a coroutine on a threadpool with a time limit or a token for cancellation. But there is a desire to make a new scheduler that would not use epoll for its work, but would use threads.