danielgerlag / workflow-core

Lightweight workflow engine for .NET Standard
MIT License
5.33k stars 1.19k forks source link

Documentation for RunWorkflowSync is out of date #783

Open evolvedlight opened 3 years ago

evolvedlight commented 3 years ago

Describe the bug The documentation (in release notes) for RunWorkflowSync is out of date.

To Reproduce Try and do:

var runner = serviceProvider.GetService<ISyncWorkflowRunner>();
...
var worfklow = await runner.RunWorkflowSync("my-workflow", 1, data, TimeSpan.FromSeconds(10));

Expected behavior It to run the workflow

Additional context I'd like to contribute some documentation about how to use this, but I'd like to ensure I understand how this is supposed to work.

For example, if we had this before: var flowId = await _workflowService.StartWorkflow(Workflows.Workflow1, model);

What does this get replaced by for the sync run?

I tried: var workflow = await _syncWorkflowRunner.RunWorkflowSync(Flows.ProductIssuanceV2Flow, 2, model, "", TimeSpan.FromSeconds(10)); But that wasn't correct, and I'm not sure exactly what's expected here. For example, after this runs I don't have any subscriptions. It also took a suprisingly long time to execute, but maybe that's fine.

And I assume similar is true for the Sync version of the PublishEvent call?

Thanks very much for your help, sorry that my many dotnet 5 versions didn't all work :(

danielgerlag commented 3 years ago

Did you start the workflow host?

evolvedlight commented 3 years ago

@danielgerlag sorry, somehow missed your message. I will check that but I believe I did start the workflow host. I assume I should have started it? Or not?

evolvedlight commented 3 years ago

Hi @danielgerlag

Ok, so I'm now in a position that I have it all working, but I think the sync workflow runner is a little broken, but I think if you confirm my suspicions, then I can contribute a patch.

Our code wasn't working for two reasons:

So I looked at what the async runner did, and I think the sync version is missing these:

foreach(var sub in result.Subscriptions)
{
    await SubscribeEvent(sub, _persistenceStore);
}
await _persistenceStore.PersistErrors(result.Errors);

and in the while loop: add the conditionworkflow.NextExecution.HasValue

Would you agree with this?

VictorioBerra commented 2 years ago

Regarding documentation, can we add RunWorkflowSync to the ReadTheDocs? I had to google, and search the issue tracker for RunWorkflowSync to find this issue to learn that theres a very short explanation on RunWorkflowSync buried in the release notes. This steps from my cancellation token not being passed to my workflow.