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

Make transition actions async-first #452

Open
wants to merge 27 commits into
base: dev
Choose a base branch
from

Conversation

sunghwan2789
Copy link

@sunghwan2789 sunghwan2789 commented May 22, 2021

This PR removes many duplicate codes that support both synchronous and asynchronous transition actions. It is done by introducing EventCallback, part of ASP.NET Core, prioritizing asynchronous code, and making synchronous code proxy it. As a result, most public APIs are acting as-is, but some are changed to support more async way.

Added

  • FireAsync() can execute sync transition actions, such as OnEntry
  • Add some missing APIs:
public class StateMachine<TState, TTrigger>
{
  public Task FireAsync(TriggerWithParameters trigger, params object[] args);

  public class StateConfiguration
  {
    public StateConfiguration OnEntryFromAsync(TriggerWithParameters trigger, Func<Transition, Task> entryAction, string entryActionDescription = null);
  }
}

Changed

  • OnTransitionedEvent invoke actions in order, regardless of async or sync

  • On*Async await transition actions, regardless of FireAsync() or Fire()

    For synchronous non-blocking transition actions, use On* and execute Task.Run() in an action.

Fixed

  • Handle OnEntryFromAsync on generating graphs

@sunghwan2789 sunghwan2789 changed the title Make code async-first Make trigger actions async-first May 23, 2021
@sunghwan2789 sunghwan2789 changed the title Make trigger actions async-first Make transition actions async-first May 23, 2021
We need TransitionConfiguration for InternalTransitionAsyncIf,
PermitDynamicAsyncIf, etc...
{
readonly TState _state;
private readonly EventCallback _callback;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use EventCallback is not enought good solution. library can used in simple console host application that does not have this dependency

@HenningNT
Copy link
Contributor

Firstly, I must say I appreciate your hard work!

I'll take a look at this when I have time, as it changes quite a lot of files.

Pentiva added a commit to Pentiva/stateless that referenced this pull request May 19, 2022
Pentiva added a commit to Pentiva/stateless that referenced this pull request Sep 2, 2022
@leeoades
Copy link
Contributor

I know this was done a while ago but I like the idea of it a lot.

(I checked out the PR and obviously, the master branch has moved on since then so I wasn't trying to rebase it.)

I agree with the sentiment completely. It would remove all of the sync and async duplication.

My initial thoughts were that instead of needing EvenCallback, we could simply wrap all sync actions like so:
(plus the argument versions obviously)

public static class ActionExtensions
{
    public static Task ToAsync(this Action action)
    {
        Task SyncWrapper(Action sync)
        {
            sync();
            return Task.CompletedTask;
        }

        return SyncWrapper(action);
    }
}

and then you would treat everything as async internally.
I believe the only thing to deal with then is someone calling Fire() instead of Fire.Async but I'm not sure a consumer would define async delegates but then call the sync version of Fire. But I think this can just use a Task .Wait().

The complication I hadn't considered was how EvenCallback deals with old skool Tasks and returns TaskResult. I need to get my head around supporting the older versions of .net pre the async await days.

I would be happy to give this another go though.

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

Successfully merging this pull request may close these issues.

4 participants