Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for async EventListener DSL #35

Merged
merged 3 commits into from Dec 11, 2021
Merged

Add support for async EventListener DSL #35

merged 3 commits into from Dec 11, 2021

Conversation

pambrose
Copy link
Contributor

@pambrose pambrose commented Dec 8, 2021

No description provided.

@holgerbrandl
Copy link
Owner

Thanks @pambrose for the great suggestion. My first experiment to enable async event consumption clearly lacked beauty. Your PR is much cleaner.

The only concern I'd like to raise is the limitation to existing event types. In the simulations we are doing, we typically create lots of other custom types which we can consume very selectively. E.g.

class ToolEvent(
    val tool: Tool,
    val processId: UUID,
    val start: TickTime,
    val end: TickTime
) :  Event(end)

val toolEvents  = collect<ToolEvent>() // which will be of type List<ToolEvent>

Actually, in some simulations we exclusively monitor the custom events and do not mind about the built-in event types, which can for performance sometimes even be suppressed via https://www.kalasim.org/advanced/#continuous-simulation

So, I wonder if the entire event API should support/follow a reified minimalistic approach for in-sync and async listeners?

addEventListener<ToolEvent>{ print(it) }
addAsyncEventListener<ToolEvent>(myOptionalScope){ print(it) }

Or maybe even just addListener<T??

Clearly, the event API should follow what's best-practise in kotlin, so that new users feel at home right very quickly.

How do you feel about that?

@holgerbrandl holgerbrandl self-assigned this Dec 8, 2021
@holgerbrandl holgerbrandl added the enhancement New feature or request label Dec 8, 2021
@holgerbrandl holgerbrandl added this to the v0.8 milestone Dec 8, 2021
@pambrose
Copy link
Contributor Author

pambrose commented Dec 8, 2021

Ah, very interesting. I had not considered custom types. Let me rework it a bit.

@pambrose
Copy link
Contributor Author

Hi Holger,

I played around with using generics and I can see 4 ways of dealing with it. I wanted to see what you think.
Here are examples of each:
1.

addAsyncEventListener<ComponentStateChangeEvent>{ event: ComponentStateChangeEvent ->
    println("onComponentStateChangeEvent: $event")
}
addEventListener(
    asyncEventListener<ComponentStateChangeEvent> { event: ComponentStateChangeEvent ->
        println("onComponentStateChangeEvent: $event")
    }
)
addEventListener(
    asyncEventListener<ComponentStateChangeEvent> { 
        onEvent { event: ComponentStateChangeEvent ->
            println("onComponentStateChangeEvent: $event")
        }
    }
)
addEventListener(
    asyncEventListener { 
        onEvent<ComponentStateChangeEvent> { event: Event ->
            println("onComponentStateChangeEvent: $event")
        }
    }
)

Pros and cons that come to mind are:

#1 is the most terse, but it creates a new method that overlaps with the existing addEventListener().
I would prefer to not see the duplication.

#2 is almost as terse, preserves the existing addEventListener() call and will not break any code.
It also allow us to offer other versions of listeners, e.g., sync, verbose, whatever, using the same
addEventListener() call.

#3 allows for multiple onEvent() calls to be specified, but they would all correspond to the same type of Event (within that scope). I am not sure multiple onEvent calls is valuable in practice.

#4 allows for mixing multiple types of onEvent() calls, but because of inline restrictions in the implementation, the argument of each onEvent() call is only Event and not the T one would want. That is not very cool.

#3 and #4 are likely providing unnecessary flexibility that would not be used, but I wanted to at least mentioned them in case you thought they might be valuable in actual usage of the API.

#1 and #2 look clean to me and I like not breaking code, so my favorite is probably #2, but I will happily defer to whichever you judge to be best.

@pambrose
Copy link
Contributor Author

I was just playing with doing a synchronous listener for #2 and I think I like offering both options using the same addEventListner() call:

addEventListener(
    asyncEventListener<ComponentStateChangeEvent> { event: ComponentStateChangeEvent ->
        println("onComponentStateChangeEvent: $event")
    }
)

and

addEventListener(
    syncEventListener<ComponentStateChangeEvent> { event: ComponentStateChangeEvent ->
        println("onComponentStateChangeEvent: $event")
    }
)

@holgerbrandl
Copy link
Owner

My favourite would be clearly (1) with quite a margin, as it's most concise and very readable. I don't mind the breaking change, since we/I have pointed on multiple occasions in the documentation (release notes, changelog, etc.) that the API is not yet stable.

Starting with the first major release, API stability will be for sure something to weigh carefully against cosmetical improvements.

@pambrose
Copy link
Contributor Author

pambrose commented Dec 10, 2021

I added an implementation of #1 and I like it. The only catch is the eventListeners value has to be public because addEventListener() is inlined and cannot reference private values. Also, I did not include async in the method name because I am assuming one would not want a synchronous version messing with the timing. Let me know if that is a bad assumption.
Also, this does not break the existing API, so we are fine there.

@holgerbrandl holgerbrandl merged commit 761dcf0 into holgerbrandl:master Dec 11, 2021
@pambrose pambrose deleted the AsyncEventListenerDsl branch December 11, 2021 17:48
@holgerbrandl
Copy link
Owner

I've slightly reworked the implementation, managed to make the listeners private again, simplified the docs, and added a test org.kalasim.test.EnvTests#it should consume events asynchronously. This test fails at the moment. The console logs are


time      current               receiver              action                                                 info                               
--------- --------------------- --------------------- ------------------------------------------------------ ----------------------------------
.00                             main                  Created
.00                             ComponentGenerator.1  Created
.00                                                   Activated, scheduled for .00                           New state: scheduled
.00                             main                  Running +5.00, scheduled for 5.00                      New state: scheduled
.00       ComponentGenerator.1  ComponentGenerator.1  Hold +1.00, scheduled for 1.00                         New state: scheduled
1.00                            Car.0                 Created
1.00                            ComponentGenerator.1  Hold +1.00, scheduled for 2.00                         New state: scheduled
2.00                            Car.1                 Created
2.00                            ComponentGenerator.1  Hold +1.00, scheduled for 3.00                         New state: scheduled
3.00                            Car.2                 Created
3.00                            ComponentGenerator.1  Hold +1.00, scheduled for 4.00                         New state: scheduled
4.00                            Car.3                 Created
4.00                            ComponentGenerator.1  Hold +1.00, scheduled for 5.00                         New state: scheduled
received event {"current":"ComponentGenerator.1","receiver":"ComponentGenerator.1","action":"hold +1.00, scheduled for 4.00","details":"New state: scheduled","time":"3.00"}

So the debug print (to be removed for sure) in

println("received event ${event}")
is only called once. Only the first event is consumed by the async listener. Do you have an idea why?

I would have a second related question. Would the coroutine Channel API allow to add a second consumer as shown in the commented code in the mentioned test implementation? Or can a flow be consumed only once?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants