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

It is no longer clear how execution::simulate_until() should work with indeterminate end time #694

Closed kyllingstad closed 2 years ago

kyllingstad commented 2 years ago

This is related to the recent removal of fibers from the code, in particular from the cosim::execution API. It is no longer clear to me how the following code should work, or if it is at all valid:

auto exe = cosim::execution( /* ... */ );
// Add stuff to the execution
// ...
auto stopped = exe.simulate_until(std::nullopt_t);

In the old version, simulate_until() was an async function that returned immediately, and its return value was a future which could be queried for the simulation status.

Now that it's no longer async, should it even be possible to pass it an indeterminate end time? Won't it just hang? One may think that one could call exe.stop_simulation() from some other thread, but the code is not thread safe. (stop_simulation() simply sets a non-atomic, non-mutex-protected bool variable.)

Running the simulation asynchronously until it's stopped by the user is a real use case. I believe the Demo App does it. So I think this needs to be cleared up pretty quickly.

kyllingstad commented 2 years ago

@markaren, you probably have the most insight into this. Any ideas?

markaren commented 2 years ago

On top of my head I suggest to remove the option. Rather, consumers of libcosim should spawn a new thread on their side for handling early abortion etc.. I mean the reason this has not surfaced is becouse libcosim does not directly utilize the feature.

Alternativly, we spawn a thread and return a future using the std::promise api. Perhaps even move this feature into it's own header.

markaren commented 2 years ago

I see that file_observer_dynamic_logging_test spawns a new thread on the client side (even prior to the fiber change).

kyllingstad commented 2 years ago

I mean the reason this has not surfaced is becouse libcosim does not directly utilize the feature.

libcosim doesn't, but libcosimc does. And right now it uses it wrongly, i.e., in a thread-unsafe manner.

markaren commented 2 years ago

I think the linked PR should suit everyone.