holgerbrandl / kalasim

Discrete Event Simulator
https://www.kalasim.org/
MIT License
68 stars 10 forks source link

Fix async event issue #39

Closed pambrose closed 2 years ago

pambrose commented 2 years ago

This fixes the async even problem by using a runBlocking context to submit the events.

With regard to your second question, yes, I believe a flow can be collected only once. I think I know what you are getting at with your question.

If you allow for multiple listeners to be added in the same call then you will be able to share the flow. Something like:

addAsyncEventListeners {
    onEvent<EntityCreatedEvent> { ... }
    onEvent<MetricEvent> { ... }
    onEvent<TimerEvent> { ... }
}
holgerbrandl commented 2 years ago

Thanks for the quick fix.

In fact, such an elaborate API has not yet crossed my mind. I'm still a newbie when it comes to coroutines. To keep it simple (and assuming that flow creation is not too expensive), I guess the user could for now just do

addAsyncEventListener<EntityCreatedEvent> { ... }
addAsyncEventListener<MetricEvent> { ... }
addAsyncEventListener<TimerEvent> { ... }
pambrose commented 2 years ago

Yeah, I do not think users will create a huge number of listeners, so each listener getting its own channel is likely fine.

pambrose commented 2 years ago

It seems the test is timing out. I am seeing that locally as well. Have you tried the tests locally?

holgerbrandl commented 2 years ago

Yes, and it fails as well if I run it together with the rest of the test-suite. The error is

Failed to add event {"current":"ComponentGenerator.1","receiver":"ComponentGenerator.1","action":"hold +1.00, scheduled for 4.00","details":"New state: scheduled","time":"3.00"} to channel
4.00                            Car.3                 Created
adding event {"time":"4.00","type":"EntityCreatedEvent"} to channel
result: Value(Failed)
Failed to add event {"time":"4.00","type":"EntityCreatedEvent"} to channel
4.00                            ComponentGenerator.1  Hold +1.00, scheduled for 5.00                         New state: scheduled
adding event {"current":"ComponentGenerator.1","receiver":"ComponentGenerator.1","action":"hold +1.00, scheduled for 5.00","details":"New state: scheduled","time":"4.00"} to channel
result: Value(Failed)
Failed to add event {"current":"ComponentGenerator.1","receiver":"ComponentGenerator.1","action":"hold +1.00, scheduled for 5.00","details":"New state: scheduled","time":"4.00"} to channel

So it seems to struggle to add the events to the channel.

Interestingly this works for me locally, when just running this (or a small subset of tests). Not sure how this can happen.

pambrose commented 2 years ago

That is odd. Let me look at it in the AM. I suspect we are leaking resources by not shutting down the coroutine.

pambrose commented 2 years ago

If I comment out the new async event listener test and run all the tests, I am still not getting a clean test run. Are you seeing that as well?

pambrose commented 2 years ago

Ah, I guess I need to have R installed. Can you confirm you are getting a clean run with R installed.

pambrose commented 2 years ago

Okay, I installed R and that improved the results. The one that seems to be funky is the first test in DepletableResourceTests. If I run it alone, it is fine. If I run it along with the others, it hangs. And that is with the async event listener test commented out.

holgerbrandl commented 2 years ago

I've just (see commit above) made all test that depend on R conditional, so that they won't run by default; I've also set some recently added tests which are still known to fail at the moment to @Ignore.

Except for the async listeners I now get a clean test run: image