stampzilla / gozwave

An opensource golang lib for zwave-controller communication thru serial-api. Tested and developed with a usb connected AEOTEC Z-Stick Gen5
Apache License 2.0
54 stars 13 forks source link

data race crash #9

Open bradfitz opened 7 years ago

bradfitz commented 7 years ago

In Go 1.8, the runtime has improved always-on data race detection for maps (see https://beta.golang.org/doc/go1.8#mapiter).

It caught this bug, which crashes the whole program instead of letting memory be silently corrupted:

root@raspberrypi:/home/pi# ./zwave-example -port=/dev/ttyUSB0
[GIN-debug] [WARNING] Running in "debug" mode. Switch to "release" mode in production.
 - using env:   export GIN_MODE=release
 - using code:  gin.SetMode(gin.ReleaseMode)

[GIN-debug] GET    /nodes                    --> main.nodes.func1 (3 handlers)
[GIN-debug] GET    /nodes/:id/:action        --> main.control.func1 (3 handlers)
[GIN-debug] Environment variable PORT is undefined. Using port :8080 by default
[GIN-debug] Listening and serving HTTP on :8080
INFO[0000] Sending: 01030002fe
INFO[0000] Recived: 06
INFO[0000] Recived: 0125010205001defffffffffff0700000000000000000000000000000000000000000000030100
WARN[0000] Started identification on node 15
WARN[0000] Started identification on node 6
Event: events.NodeDiscoverd{Address:4, FuncGetNodeProtocolInfo:functions.FuncGetNodeProtocolInfo{Listening:false, Routing:false, Beaming:false, Version:0x0, Flirs:false, Security:false, MaxBaud:0, Basic:0x0, Generic:0x0, Specific:0x0, data:[]uint8(nil)}}
fatal error: concurrent map read and map write

goroutine 1 [running]:
runtime.throw(0x508ac3, 0x21)
        /home/bradfitz/go/src/runtime/panic.go:596 +0x70 fp=0x10a31edc sp=0x10a31ed0
runtime.mapaccess1_fast32(0x4a2dd8, 0x10aeda20, 0x4, 0x0)
        /home/bradfitz/go/src/runtime/hashmap_fast.go:21 +0x19c fp=0x10a31eec sp=0x10a31edc
github.com/stampzilla/gozwave/nodes.List.Get(0x10aeda20, 0x10a17130, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x4, 0x0)
        /home/bradfitz/src/github.com/stampzilla/gozwave/nodes/list.go:50 +0x9c fp=0x10a31f08 sp=0x10a31eec
main.main()
        /home/bradfitz/src/github.com/stampzilla/gozwave/zwave-example/example.go:38 +0x3b4 fp=0x10a31fbc sp=0x10a31f08
runtime.main()
        /home/bradfitz/go/src/runtime/proc.go:185 +0x1e4 fp=0x10a31fe4 sp=0x10a31fbc
runtime.goexit()
        /home/bradfitz/go/src/runtime/asm_arm.s:1017 +0x4 fp=0x10a31fe4 sp=0x10a31fe4
stamp commented 7 years ago

I added some Lock/Unlock calls and I think I managed to fetch most of the race conditions.

But I don't like that solution :/ I think we return pointers to often. Maybe there are an other better pattern to use?

jonaz commented 7 years ago

You should only lock at the outer most level ( using getters and setters) where you are modifying shared maps. Seems like you added alot of other locks which are not needed?

stamp commented 7 years ago

Its possible that I added to many. The problem is that there are many gorutines that uses the same map. Most of then are created by the Identify() method in nodes/nodes.go

If you checkout the d021d0e538a32e34d92d893fe58ac310d67e7983 commit an run the example with go run -race *.go you will see a lot of race conditions