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

Refactor 'autorun' handling into a loop (from recursion) #110

Merged
merged 2 commits into from
Jul 29, 2021

Conversation

ehuelsmann
Copy link
Member

Description

Very long workflows can cause deep stacks; OpenXPKI is experiencing
negative effects from this. However, in fact the situation is that
'autorun' can just as easily be handled with a loop.

This change factors most of 'execute_action' into the private function
'_execute_single_action' and incorporates '_auto_execute_state' into
'execute_action', which is now a loop which runs as long as the autorun
requirements are satisfied.
Fixes/addresses (If applicable) # (issue)

Type of change

Please delete options that are not relevant.

  • Optimization

Checklist:

  • My code follows the style guidelines of this project, please see the contribution guidelines.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Very long workflows can cause deep stacks; OpenXPKI is experiencing
negative effects from this. However, in fact the situation is that
'autorun' can just as easily be handled with a loop.

This change factors most of 'execute_action' into the private function
'_execute_single_action' and incorporates '_auto_execute_state' into
'execute_action', which is now a loop which runs as long as the autorun
requirements are satisfied.
@coveralls
Copy link

coveralls commented Apr 23, 2021

Coverage Status

Coverage decreased (-0.3%) to 84.665% when pulling ccb0763 on eliminate-autorun-recursion into c4e9d9c on master.

@ehuelsmann
Copy link
Member Author

@oliwel remarks on Slack that we may need to mark this 2.0 as it will break the OpenXPKI functionality which depends on the fact that execute_action() is called for every step in the workflow -- which this no longer does...

@ehuelsmann
Copy link
Member Author

While OpenXPKI depends on the fact that each executed step (including the autorun steps) is executed using $wf->execute_action(), I'm not sure the interface guarantees it. So, while this change breaks OpenXPKI, I'm not sure this is a 2.0 change... @oliwel wants to make this happen, but had different changes in mind. Lets discuss on Slack more.

@jonasbn jonasbn added this to the 2.0 milestone Apr 25, 2021
@jonasbn jonasbn added breakingchange enhancement Feature/improvement and removed deprecation labels Jul 15, 2021
Copy link
Collaborator

@jonasbn jonasbn left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Collaborator

@jonasbn jonasbn left a comment

Choose a reason for hiding this comment

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

This looks good, more testing is possibly required

@jonasbn jonasbn merged commit e120445 into master Jul 29, 2021
@delete-merged-branch delete-merged-branch bot deleted the eliminate-autorun-recursion branch July 29, 2021 10:26
$self->_factory()->save_workflow($self);

# If using a DBI persister with no autocommit, commit here.
$self->_factory()->_commit_transaction($self);
Copy link
Member Author

Choose a reason for hiding this comment

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

I've been wondering since this refactoring: shouldn't there be an observer message which the persister can hook into in order to commit its transaction?

);
$self->state($old_state);

$self->_factory()->_rollback_transaction($self);
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here, but notify the failure?

lib/Workflow.pm Outdated
@@ -377,6 +378,30 @@ sub _get_next_state {
return $wf_state->get_next_state( $action_name, $action_return );
}

sub _auto_execute_state {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll submit a follow-up PR: This function was modified on the branch you merged in, but removed on the PR. The modified version is - just as the original - irrelevant after the changes in the PR. I'll follow up with a PR to remove this function.

@ehuelsmann ehuelsmann mentioned this pull request Jul 29, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants