open-simulation-platform / libcosim

OSP C++ co-simulation library
https://open-simulation-platform.github.io/libcosim
Mozilla Public License 2.0
55 stars 10 forks source link

Async simulate_until #696

Closed markaren closed 2 years ago

markaren commented 2 years ago

Less intrusive and simpler alternative to #695

Resolves #694

restenb commented 2 years ago

Yes, this is what I had in mind as well, a minimal implementation that fixes both issues noted by @kyllingstad in #694.

With the addition of simulate_until_async, I guess there's no need for the argument to simulate_untilto keep being optional - though maybe requiring a cosim::time_point is just "unnecessary" API breakage anyway?

kyllingstad commented 2 years ago

Maybe we can do it even simpler: Leave simulate_until() as is, keeping the optional argument, and just make stop_simulation() threadsafe as done here. Then we can specify in the documentation that simulate_until({}) will block the current thread indefinitely, but that it is safe to stop it from another thread. (The fact that stop_simulation() is threadsafe should be specified in its documentation too.)

Then the user can choose how to implement the multithreading. Maybe they want to use std::async() for simplicity, but they could also prefer to have more control over the threading. (For example, some applications may have a business thread and a UI thread, or use a custom thread pool, or use boost::thread rather than std::thread, etc. etc. etc.).

markaren commented 2 years ago

The existing signature of simulate_until (rather than void and non-optional parameter) was kept in order for clients to override the behaviour of the async version. I still think there is value in adding a default non-blocking function.

restenb commented 2 years ago

I'm OK with keeping the built-in simulate_until_async option, clients that want to handle threading can use the blocking version. This could also be noted in the documentation.

I have one code comment: On line 119 of execution.cpp , we return !stopped_. I assume this has to be changed to return !stopped_.load() to ensure thread safety also for the is_running() function.

markaren commented 2 years ago

According to stackoverflow load/store is already being called implicitly.