antoniodipinto / ikisocket

🧬 WebSocket wrapper with event management for Fiber https://github.com/gofiber/fiber. Based on Fiber WebSocket and inspired by Socket.io
MIT License
123 stars 21 forks source link

Race condition for pool, listeners and kws #10

Closed dmorawetz closed 3 years ago

dmorawetz commented 3 years ago

go run -race is reporting data races in your implementation. The pool and listeners maps should each have a sync.RWMutex. Furthermore, I would place RWMutex in Websocket struct and lock it, before changing any of the fields.

You could also write something like

type Websocket struct {
        sync.RWMutex
    // The Fiber.Websocket connection
    ws *websocket.Conn
    ...
}

...

kws.Lock()
...
kws.Unlock()
antoniodipinto commented 3 years ago

Hi, thank you for this message. Since ikisocket is a wrapper, the real handling of the socket is made in Fiber and this cannot be tested directly with -race. I will take a look into your proposal, makes sense ofc, I hope this will not have repercussions on Fiber Websocket implementation

dmorawetz commented 3 years ago

-race is at least complaining about the read and write methods. I am guessing pool and listeners might be at risk, because fiber is handling requests in parallel. So when there are e.g. two new connections at the same time, both handlers might try to write into pool.

dmorawetz commented 3 years ago

fatal error: concurrent map writes

goroutine 9 [running]: runtime.throw(0x73994e, 0x15) /usr/lib/go/src/runtime/panic.go:1117 +0x72 fp=0xc000076d88 sp=0xc000076d58 pc=0x439cf2 runtime.mapassign_faststr(0x6dfa20, 0xc00033a270, 0xc00038e0e0, 0x64, 0xc000390000) /usr/lib/go/src/runtime/map_faststr.go:211 +0x3f1 fp=0xc000076df0 sp=0xc000076d88 pc=0x4161f1 github.com/antoniodipinto/ikisocket.New.func1(0xc00033b830) /home/daniel/Projects/ikisocket/ikisocket.go:140 +0x1e5 fp=0xc000076e60 sp=0xc000076df0 pc=0x6a8a85

I was able to provoke the runtime error in a unit test.

antoniodipinto commented 3 years ago

Good to know :)

Nothing like that happened in my tests, but it's ok, you can implement it by yourself if you want and create a pull request.

Thank you for your interest