Event engine implementation #139

parfeon · 2023-05-18T06:48:22Z

feat(ee): event engine implementation

Provide generic event engine implementation which can operate over different states.

feat(ee): provide subscribe states

Provide a list of states which correspond to subscribe state machine.

feat(ee): provide list of subscribe state machine events

Provide a list of events which trigger transition between states in subscribe state machine.

feat(ee): define transitions

Define transition directions in response to event, depending on from current state machine state.

feat(ee): provide list of effects and invocations

Provide a list of effects invocation and effects which can be called as part of transitions.

Provide generic event engine implementation which can operate over different states. feat(ee): provide subscribe states Provide list of states which correspond to subscribe state machine. feat(ee): provide list of subscribe state machine events Provide list of events which trigger transition between states in subscribe state machine. feat(ee): define transitions Define transition directions in response of event depending from current state machine state. feat(ee): provide list of effects and invocations Provide list of effects invocation and effects which can be called as part of transitions.

parfeon · 2023-05-18T06:49:12Z

FYI: Clippy won't be happy with this because a lot of code detached (it needs to be bound with actual effect implementation and EE usage).

Xavrax · 2023-05-18T08:14:03Z

You can add #[allow(dead_code)] for the whole modules.

src/core/event_engine/effect.rs

src/core/event_engine/effect_dispatcher.rs

src/core/event_engine/effect_invocation.rs

src/core/event_engine/mod.rs

src/dx/subscribe/event_engine/event.rs

refactor(ee): make EE work with `no_std`

Xavrax

Overall LGTM but I have some questions.

src/core/event_engine/effect_dispatcher.rs

Xavrax · 2023-05-22T05:48:24Z

src/core/event_engine/mod.rs

+        let mut writable_state = self.current_state.write();
+        *writable_state = transition.state;
+        drop(writable_state);


This pattern is also pretty common in rust community:

Suggested change

let mut writable_state = self.current_state.write();

*writable_state = transition.state;

drop(writable_state);

{

let mut writable_state = self.current_state.write();

*writable_state = transition.state;

}

I prefer that one above but take this one which is preferred by you.

Made this change in recent push.

src/core/event_engine/state.rs

src/core/event_engine/effect_handler.rs

src/core/event_engine/effect_invocation.rs

Xavrax

What thing makes me worried. There are 1.5k lines without any test ;o

Xavrax

Some question that I found during working with effects.

Xavrax · 2023-05-22T09:55:14Z

src/core/event_engine/effect_dispatcher.rs

+        F: Fn(Vec<E>),
+    {
+        if let Some(effect) = self.handler.create(invocation) {
+            let effect = Rc::new(effect);


Why do we need that RC here?
Why do we passing the remove_managed_effect into the effect and call it inside of it?

Additionally, if we need reference counter it probably won't work with our client as it needs to be thread safe.
Use Arc instead.

Why do we need that RC here?

I have a requirement to store managed effect in a vector and make it possible to remove the same effect from list after run completion.
I tried to filter in completion block, but borrow checker won't let me go this way. Furthermore, I used Rc just to make clone once to add it vector and don't need Rc functionality after that.

Why do we passing the remove_managed_effect into the effect and call it inside of it?

I can't follow this one.
The idea was that run function of effect has completion closure, with which we will be able to remove effect from the list of managed effects.

Additionally, if we need reference counter it probably won't work with our client as it needs to be thread safe.
Use Arc instead.

Rc functionality used once (I mean clone) within dispatch function, and that is all. All other access to it through vector which stores it should be safe enough because of RwLock.

The idea was that run function of effect has completion closure, with which we will be able to remove effect from the list of managed effects.

This one is still confusing to me.
How effects are allowed to managed the call?
Currently in SubscribeEffect it doesn't make any decision about it. Do you think that some effects will affect that?
We still can casually call that line after calling run function.

parfeon · 2023-05-22T16:43:14Z

What thing makes me worried. There are 1.5k lines without any test ;o

I will work on this.

Xavrax · 2023-05-24T12:43:37Z

src/core/event_engine/effect_dispatcher.rs

+        F: Fn(Vec<E>),
+    {
+        if let Some(effect) = self.handler.create(invocation) {
+            let effect = Rc::new(effect);


The idea was that run function of effect has completion closure, with which we will be able to remove effect from the list of managed effects.

This one is still confusing to me.
How effects are allowed to managed the call?
Currently in SubscribeEffect it doesn't make any decision about it. Do you think that some effects will affect that?
We still can casually call that line after calling run function.

Xavrax · 2023-05-24T12:44:49Z

src/core/event_engine/effect_dispatcher.rs

+                // Notify about effect run completion.
+                // Placeholder for effect events processing (pass to effects handler).
+                f(events);


So these lines assume's that f() have to be called after the Effect.
As it's synchronous - I still don't see sense of that call. Maybe Am I missing something ;o?

We don't have implemented effects yet, but I assume that some of them would return events, which should be processed by the event engine.
It is exactly as it says – it is a placeholder for functionality, which I expect to appear with implemented effects.
The function pointer can be passed to the created effect, so it will be called from it.

I'm still not sure about that.
Anyway imo you can merge that.
Rest of code looks good.

src/dx/subscribe/event_engine/state.rs

Xavrax

LGTM (except that part with function)

parfeon self-assigned this May 18, 2023

parfeon requested review from Xavrax, kleewho, marcin-cebo and mohitpubnub as code owners May 18, 2023 06:48

Xavrax reviewed May 18, 2023

View reviewed changes

parfeon and others added 4 commits May 19, 2023 11:15

refactor(ee): apply changes according to review

208cc3f

refactor(ee): make EE work with `no_std`

refactor(ee): fix issues reported by clippy

fa61bd9

refactor(cargo): disable 'phantom-type' default features

26e8fdc

Merge branch 'phoenix' into clen-1219/feat/add-ee-transitions

5c48cf0

Xavrax reviewed May 22, 2023

View reviewed changes

test(ee): add event engine tests

8ffc5ec

Xavrax reviewed May 24, 2023

View reviewed changes

refactor(ee): use test_case for state transitions tests

2915233

Xavrax approved these changes May 24, 2023

View reviewed changes

Xavrax approved these changes May 25, 2023

View reviewed changes

Xavrax merged commit 665b518 into phoenix May 25, 2023

Xavrax deleted the clen-1219/feat/add-ee-transitions branch May 25, 2023 09:48

Event engine implementation #139

Event engine implementation #139

Uh oh!

Conversation

parfeon commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parfeon commented May 18, 2023

Uh oh!

Xavrax commented May 18, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xavrax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Xavrax May 22, 2023

Choose a reason for hiding this comment

Uh oh!

parfeon May 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xavrax left a comment

Choose a reason for hiding this comment

Uh oh!

Xavrax left a comment

Choose a reason for hiding this comment

Uh oh!

Xavrax May 22, 2023

Choose a reason for hiding this comment

Uh oh!

parfeon May 22, 2023

Choose a reason for hiding this comment

Uh oh!

Xavrax May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parfeon commented May 22, 2023

Uh oh!

Xavrax May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xavrax May 24, 2023

Choose a reason for hiding this comment

Uh oh!

parfeon May 24, 2023

Choose a reason for hiding this comment

Uh oh!

Xavrax May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Xavrax left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

parfeon commented May 18, 2023 •

edited

Loading

Xavrax May 24, 2023 •

edited

Loading

Xavrax May 24, 2023 •

edited

Loading

Xavrax May 24, 2023 •

edited

Loading