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

Fix test error description and add cancel transaction method #77

Closed
wants to merge 4 commits into from

Conversation

suerta-git
Copy link

@suerta-git suerta-git commented Sep 28, 2021

⚠️ May breaking changes

Extended the transitioner interface with cancelTransition(*FSM) error method.

@suerta-git suerta-git changed the title Fix test error description Fix test error description and add cancel transaction method Sep 28, 2021
@suerta-git
Copy link
Author

Relating to #76

Copy link
Member

@maxekman maxekman 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! But would be like the nicely written doc comment to public.

@@ -377,6 +390,18 @@ func (t transitionerStruct) transition(f *FSM) error {
return nil
}

// CancelTransition cancels an asynchrounous state change.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is great! But it will never be visible as this is a private method on a private struct. Can you move it to the public CancelTransition() method? If you can also update the public Transition() comment to match this quality it would be a welcome bonus!

@maxekman
Copy link
Member

Please also rebase as I just merged a small typo fix PR.

@maxekman
Copy link
Member

@suerta-git What is your reasoning behind adding the context to the AsyncError? Wasn't the previous solution you did ok?

@suerta-git
Copy link
Author

suerta-git commented Sep 29, 2021

Please also rebase as I just merged a small typo fix PR.

Sorry @maxekman this forked repo is just my own experimental implementation so it contains too much thing. I will raise another formal one for specific change.

@suerta-git
Copy link
Author

@suerta-git What is your reasoning behind adding the context to the AsyncError? Wasn't the previous solution you did ok?

The reason of adding context is I hope there is a way can let me know the transition has been canceled. So I add a context as a part of return from Event() and it can notify the cancellation.

Example:

err := fsm.Event("start")
asyncErr, ok := err.(FSM.AsyncError)
if !ok {
	// ...
	return
}

ticker := time.NewTicker(time.Second)
defer ticker.Stop()

for _, second := range []int{3, 2, 1} {
	select {
	case <-ticker.C:
		fmt.Printf("Tick:%d", second))
	case <-asyncErr.Ctx.Done():
		return
	}
}
fsm.Transition()

But in my own app there seems to be some problems...
Not sure if my idea is correct

@maxekman
Copy link
Member

I understand! Yes please continue to experiment/work until you are happy. Several PRs might be easier to review and merge. Ping me when you want a review!

annismckenzie added a commit to alfaview/fsm that referenced this pull request Dec 12, 2021
This adds a context and cancelation facility to the type `AsyncError`.
Async state transitions can now be canceled by calling `CancelTransition`
on the AsyncError returned by `fsm.Event`. The context on that error can
also be handed off as described in looplab#77 (comment).
@annismckenzie
Copy link
Contributor

@suerta-git While working on implementing #43 and #61 I stumbled up your great idea to allow async state transitions to be canceled. It's implemented in alfaview#1. Maybe you'd like to take a look?

annismckenzie added a commit to alfaview/fsm that referenced this pull request Dec 12, 2021
This adds a context and cancelation facility to the type `AsyncError`.
Async state transitions can now be canceled by calling `CancelTransition`
on the AsyncError returned by `fsm.Event`. The context on that error can
also be handed off as described in looplab#77 (comment).
annismckenzie added a commit to alfaview/fsm that referenced this pull request Dec 12, 2021
This adds a context and cancelation facility to the type `AsyncError`.
Async state transitions can now be canceled by calling `CancelTransition`
on the AsyncError returned by `fsm.Event`. The context on that error can
also be handed off as described in looplab#77 (comment).
annismckenzie added a commit to alfaview/fsm that referenced this pull request Dec 12, 2021
This adds a context and cancelation facility to the type `AsyncError`.
Async state transitions can now be canceled by calling `CancelTransition`
on the AsyncError returned by `fsm.Event`. The context on that error can
also be handed off as described in looplab#77 (comment).
annismckenzie added a commit to alfaview/fsm that referenced this pull request Dec 12, 2021
This adds a context and cancelation facility to the type `AsyncError`.
Async state transitions can now be canceled by calling `CancelTransition`
on the AsyncError returned by `fsm.Event`. The context on that error can
also be handed off as described in looplab#77 (comment).
annismckenzie added a commit to alfaview/fsm that referenced this pull request Dec 12, 2021
This adds a context and cancelation facility to the type `AsyncError`.
Async state transitions can now be canceled by calling `CancelTransition`
on the AsyncError returned by `fsm.Event`. The context on that error can
also be handed off as described in looplab#77 (comment).
@suerta-git
Copy link
Author

@annismckenzie Nice work! It seems you do a great enhancement for current implementation. Looks good for me.

@maxekman Since I have refactored my application and no longer need to cancel transitions, I will close this PR. Thanks.

@suerta-git suerta-git closed this Dec 13, 2021
annismckenzie added a commit to alfaview/fsm that referenced this pull request Jul 29, 2022
This adds a context and cancelation facility to the type `AsyncError`.
Async state transitions can now be canceled by calling `CancelTransition`
on the AsyncError returned by `fsm.Event`. The context on that error can
also be handed off as described in looplab#77 (comment).
maxekman pushed a commit that referenced this pull request Oct 6, 2022
* Allow state transitions in callbacks

This adds the possibility of "starting" a state machine and have it
execute multiple state transitions in succession, given that no errors
occur.

The equivalent code without this change:
```go
var errTransition error

for errTransition == nil {
	transitions := request.FSM.AvailableTransitions()
	if len(transitions) == 0 {
		break
	}
	if len(transitions) > 1 {
		errTransition = errors.New("only 1 transition should be available")
	}
	errTransition = request.FSM.Event(transitions[0])
}

if errTransition != nil {
	fmt.Println(errTransition)
}
```

Arguably, that’s bad because of several reasons:
1. The state machine is used like a puppet.
2. The state transitions that make up the "happy path" are encoded
   outside the state machine.
3. The code really isn’t good.
4. There’s no way to intervene or make different decisions on which
   state to transition to next (reinforces bullet point 2).
5. There’s no way to add proper error handling.

It is possible to fix a certain number of those problems but not all
of them, especially 2 and 4 but also 1.

The added test is green and uses both an enter state and an after event
callback.

No other test case was touched in any way (besides enhancing the
context one that was added in the previous commit).

* Allow async state transition to be canceled

This adds a context and cancelation facility to the type `AsyncError`.
Async state transitions can now be canceled by calling `CancelTransition`
on the AsyncError returned by `fsm.Event`. The context on that error can
also be handed off as described in #77 (comment).

* Add example for triggering transitions in callbacks

* Add example for canceling an async transition
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.

3 participants