Implemented first stab at thread safety #14

kristoiv · 2016-10-06T15:03:32Z

Stab at resolving issue #13 . Looking for feedback.

kristoiv · 2016-10-06T15:03:54Z

Oh, and tests run fine.

maxekman

Thanks, looks nice. It would be really nice if you could add a test case as the first commit that would trigger the concurrent issue and see it pass with your changes.

maxekman · 2016-10-06T15:13:40Z

fsm.go

@@ -125,6 +129,7 @@ type Callbacks map[string]Callback
 // currently performed.
 func NewFSM(initial string, events []EventDesc, callbacks map[string]Callback) *FSM {
 	var f FSM
+	f.mutex = sync.RWMutex{}


This is not needed, it is ready to go as it is.

maxekman · 2016-10-06T15:14:23Z

fsm.go

@@ -42,6 +43,9 @@ type transitionerStruct struct {
 //
 // It has to be created with NewFSM to function properly.
 type FSM struct {
+	// mutex is used for synchronization to achieve thread safety
+	mutex sync.RWMutex


Would be cleaner to have it as a anonymous field.

If it was anonymous, wouldn't the Lock/RLock/Unlock/RUnlock methods be public? Thus external access would be dangerous.

Yes, you are correct! Keep the current approach.

maxekman · 2016-10-06T15:15:03Z

fsm.go

@@ -199,23 +204,31 @@ func NewFSM(initial string, events []EventDesc, callbacks map[string]Callback) *

 // Current returns the current state of the FSM.
 func (f *FSM) Current() string {
+	f.mutex.RLock()


With a anonymous field this would simply be f.RLock().

maxekman · 2016-10-06T15:17:24Z

fsm.go

+}
+
+// transitionInternal wraps transitioner.transition.
+func (f *FSM) transitionInternal() error {


I think doTransition() would be a better name.

…ry var declaration.

kristoiv · 2016-10-06T15:26:25Z

What do we think now?

kristoiv · 2016-10-06T15:31:55Z

Ah, I didn't remember your first comment. Creating a test-case may be difficult since it is a race condition.

One problem I see with the new implementation is access from callbacks will block if they use the public API of their own *FSM object.

maxekman · 2016-10-06T15:40:05Z

A test case using goroutine for either the read or write together with the -race flag should trigger it. See https://golang.org/doc/articles/race_detector.html

Yes, I was also thinking a bit by the possible case of deadlocking within callbacks. Not sure how to prevent that right now. Do you have ideas?

kristoiv · 2016-10-06T15:41:14Z

I have written a testcase that results in a deadlock, trying to figure out a clean way of doing it. But there will be a lot more lock/unlock calls during Event(...)'s.

maxekman · 2016-10-06T15:47:42Z

Wonderful! Let me know when I should take a look.

…eadlock

… -race

kristoiv · 2016-10-06T16:03:45Z

There! I went back to before I added thread safety and added a test proving the lack of thread safety, then I merged it to my current HEAD and confirmed the problem to be solved.

I also fixed the deadlock problem as well as I could.

maxekman

Looks good. Let me just check out the code a bit later and try it out before we merge.

kristoiv · 2016-10-06T16:22:21Z

Cool, you do that :)

…r locking public api

maxekman

Nice tests! I had a comment/idea regarding readability and maintainability of the code.

maxekman · 2016-10-06T17:27:01Z

fsm.go

@@ -306,13 +326,17 @@ func (t transitionerStruct) transition(f *FSM) error {
 // general version.
 func (f *FSM) beforeEventCallbacks(e *Event) error {
 	if fn, ok := f.callbacks[cKey{e.Event, callbackBeforeEvent}]; ok {
+		f.mutex.Unlock()


It would be less repetitive and error prone in these cases to wrap the callbacks in NewFSM at line 193 with a function that does the unlocking and locking:

f.callbacks[cKey{target, callbackType}] = func(e *Event) { // This always happens in a locked condition, but needs to be unlocked // to not risk a deadlock. The reversed order is not a mistake! f.mutex.Unlock() defer f.mutex.Lock() c(e) }

If you do this, could you also change var f FSM in the beginning to pointer form:

f := &FSM{ transitionerObj: &transitionerStruct{}, current: initial, transitions: make(map[eKey]string), callbacks: make(map[cKey]Callback), }

And return it with out dereferencing.

Do you think this would be good?

Yes definitely. I fixed both. Good call!

…cking to a single location in the code

kristoiv · 2016-10-06T20:03:32Z

Ops: I simplified my solution a little more

maxekman

Good, I was just going to suggest that. 😊

Could you also leave a comment similar to my suggestion in there?

Another minor change could be to use defer for the lock, what do you think about that?

kristoiv · 2016-10-07T07:12:17Z

I added your comment :)

In our case defering or not doesn't make any difference, but I changed it as you suggest. Arguably if the code is ever changed, whether or not it should be run in a lock or not depends on its function.

maxekman · 2016-10-07T07:15:27Z

Definitely, I can be too picky sometimes!

Do you think we should merge this now?

kristoiv · 2016-10-07T07:17:43Z

Haha, it's a good looking syntax in general to keep locks and unlocks together, so why not 👍 .
I don't see why not, looks to be a good improvement.

maxekman · 2016-10-07T07:46:30Z

Thanks for all the work with this PR!

kristoiv · 2016-10-07T08:10:20Z

No problem. Thanks for all your feedback!

Implemented first stab at thread safety

cbe76ac

Language typo

4cc9c75

maxekman requested changes Oct 6, 2016

View reviewed changes

kristoiv added 2 commits October 6, 2016 17:24

Renamed internal transition method to doTransition. Removed unnecessa…

445b264

…ry var declaration.

Merge branch 'master' of github.com:kristoiv/fsm

dd9c103

kristoiv added 3 commits October 6, 2016 17:52

Added test case for deadlock related to thread safety and fixed the d…

957055b

…eadlock

Added test to confirm a thread safety issue. Test fails when run with…

4683fd0

… -race

Merge branch 'orig'

329e87b

maxekman reviewed Oct 6, 2016

View reviewed changes

Removed double locking caused by locking public api calling on anothe…

2ff3c30

…r locking public api

maxekman reviewed Oct 6, 2016

View reviewed changes

kristoiv added 3 commits October 6, 2016 21:29

Changed the style of instantiation in NewFSM and moved unlocking/relo…

5f47b77

…cking to a single location in the code

Go fmt

cce7238

Simplified the code

3b86ae2

maxekman reviewed Oct 7, 2016

View reviewed changes

Added a comment and defered a relock instead of calling it directly

b7f67df

maxekman approved these changes Oct 7, 2016

View reviewed changes

maxekman merged commit 21fc506 into looplab:master Oct 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented first stab at thread safety #14

Implemented first stab at thread safety #14

kristoiv commented Oct 6, 2016

kristoiv commented Oct 6, 2016

maxekman left a comment

maxekman Oct 6, 2016

kristoiv Oct 6, 2016

maxekman Oct 6, 2016 •

edited

Loading

kristoiv Oct 6, 2016

maxekman Oct 6, 2016

maxekman Oct 6, 2016

maxekman Oct 6, 2016

kristoiv Oct 6, 2016

kristoiv commented Oct 6, 2016

kristoiv commented Oct 6, 2016

maxekman commented Oct 6, 2016

kristoiv commented Oct 6, 2016

maxekman commented Oct 6, 2016

kristoiv commented Oct 6, 2016

maxekman left a comment

kristoiv commented Oct 6, 2016

maxekman left a comment

maxekman Oct 6, 2016 •

edited

Loading

kristoiv Oct 6, 2016

kristoiv commented Oct 6, 2016

maxekman left a comment

kristoiv commented Oct 7, 2016

maxekman commented Oct 7, 2016

kristoiv commented Oct 7, 2016

maxekman commented Oct 7, 2016

kristoiv commented Oct 7, 2016

Implemented first stab at thread safety #14

Implemented first stab at thread safety #14

Conversation

kristoiv commented Oct 6, 2016

kristoiv commented Oct 6, 2016

maxekman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxekman Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoiv commented Oct 6, 2016

kristoiv commented Oct 6, 2016

maxekman commented Oct 6, 2016

kristoiv commented Oct 6, 2016

maxekman commented Oct 6, 2016

kristoiv commented Oct 6, 2016

maxekman left a comment

Choose a reason for hiding this comment

kristoiv commented Oct 6, 2016

maxekman left a comment

Choose a reason for hiding this comment

maxekman Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoiv commented Oct 6, 2016

maxekman left a comment

Choose a reason for hiding this comment

kristoiv commented Oct 7, 2016

maxekman commented Oct 7, 2016

kristoiv commented Oct 7, 2016

maxekman commented Oct 7, 2016

kristoiv commented Oct 7, 2016

maxekman Oct 6, 2016 •

edited

Loading

maxekman Oct 6, 2016 •

edited

Loading