search

holgerbrandl / kalasim

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

Take a executor approach to async event listener #41

Closed pambrose closed 2 years ago

pambrose commented 2 years ago

Have look at this and see what you think.

Notice we do not leak resources as long as the user calls removeEventListener()

holgerbrandl commented 2 years ago

I've fixed the implementation. There were a few flaws relating to use of runBlocking, channel-size and coroutine contexts. Now it seems fine to me, and is passing the tests.

Thanks for your help!

pambrose commented 2 years ago

A couple comments:

1) As a general rule, GlobalScope.launch is discouraged because it can leak resources. That is why I fired off a thread with newSingleThreadExecutor() instead, which I do not like either.

2) The AsyncEventListner.stop() has to be called or you will leak. I called it as part of the removeEventListener(), so it is consistent with how the other event listeners are dealt with.

holgerbrandl commented 2 years ago

Regarding (1) I had replaced GlobalScope with CoroutineScope(Dispatchers.Default) which is correct I think.

Regarding (2) I think the leak does not matter for the tests. I've kept the close method in the implementation, so the user has at least the option to close the channel.