top | item 36376439

(no title)

TheSwordsman | 2 years ago

This state machine is inherently unsafe for concurrent use, because the CurrentState and Transitions fields aren't completely protected by a mutex. You already have an unexported mutex that you lock when mutating those fields, but then consumers trying to read those exported fields are not able to lock the same mutex to prevent concurrent read/write operations.

You should not export those fields, and instead make them available via methods where the reads are protected by the mutex. You'll probably also need to make a copy of the map since it's a reference type, OR make the accessor take the key name and fetch the value directly from the map and return it. I learned this when I wrote a similar simple state machine in Go ~7 years ago. :)

I'd also make sure to return `T` from the CurrentState accessor method and not `*T`, just to make it easier for consumers to do comparison operations with the returned result.

Reference on Go memory safety: https://go.dev/ref/mem

discuss

order

hishamk|2 years ago

Thank you so much for this head's up. I've just pushed out a new update that covers this and a few other changes.