Data races in example

[anisse]

I've seen a pattern used in this example: https://github.com/hybridgroup/gobot/blob/3daa17dd99916c36bc29018d8f985da1e19321d6/examples/ardrone_ps3.go

Which is to use a variable to share the current state of sensor (here rightStick and leftStick). This is a data-race when you copy the variable. I've wrote a simple test that reproduces the same pattern:

func TestEventerDataRace(t *testing.T) {
        var r int
        e := NewEventer()

        e.On("test", func(data interface{}) {
                r++
        })

        Every(10*time.Microsecond, func() {
                x := r // this should be a race
                t.Logf("%d ...", x)
        })
        for i := 0; i < 10000; i++ {
                e.Publish("test", true)
        }
}

Running this go test -race -run DataRace detects the issue.

It would be better to communicate using channels instead of shared memory, or even use a mutex.

I haven't looked at all the examples, but it might be good to do a sweep to ensure this isn't a common pattern.

[deadprogram]

That example, and a couple others, should probably be more like this:

https://gist.github.com/deadprogram/dfe1ebf67d0b64c542a10c4260983462

@anisse what do you think?

[anisse]

@deadprogram this looks good, but you'd need to drain the channels to get the same functionality as before (use latest value). Something like this:

func getLeftStick() {
    for {
        select {
        case leftStick.x = <-leftX:
        case leftStick.y = <-leftY:
        default:
            return
        }
    }
}

And even then, it is not enough, because since channels are blocking, the function called by the stick.On Eventer will block, and you won't have the latest value all the time.

Thinking more about this, the mutex is probably the easy way out. But I'd probably use the atomic package; it doesn't support directly using float64s, but you can convert them using the math package.

[deadprogram]

That was my first implementation (muted), but then I tried channels. Just discovered atomic, interesting...

[deadprogram]

Not "muted", "mutex". Autocorrect...

[anisse]

Just discovered the abstract atomic.Value. Looks nice. I'd probably use it with the whole structure for the sake of simplicity, like this:

var leftValue atomic.Value
var leftStick pair
leftStick = pair{x: 0, y: 0}
setLeftStick(leftStick)

func setLeftStick(l pair) {
    leftValue.Store(l)
}

func getLeftStick() pair {
    return leftValue.Load()
}

I'm not sure this is correct code though. Needs more testing.

[deadprogram]

Due to how joystick sends data indepenently per axis per stick, in this case we cannot update both at the same time. But I agree this is a much nicer implementation using sync/atoomic.

Here is my take on this: https://gist.github.com/deadprogram/a103e6fe5868ef24c282e73bf472d9f7

[anisse]

@deadprogram : I like your implementation. Simple and straightforward.

[deadprogram]

@anisse I'm glad you brought up this issue. Going to do some testing, but I think this helps refine the pattern in a way that is much clearer as well as actually thread safe, not just "seems to work".

[deadprogram]

@anisse I've just pushed a commit to the dev branch with these changes.

Going to close this issue now, please reopen if you think is needed.

Thank you again!