Skip to content

Race condition in keepAlive() due to unsynchronized access to lastResponse #704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Never-Know-N opened this issue Apr 16, 2025 · 1 comment
Assignees

Comments

@Never-Know-N
Copy link

func keepAlive(c *websocket.Conn, timeout time.Duration) {
ticker := time.NewTicker(timeout)
lastResponse := time.Now()
c.SetPingHandler(func(pingData string) error {
// Respond with Pong using the server's PING payload
err := c.WriteControl(
websocket.PongMessage,
[]byte(pingData),
time.Now().Add(WebsocketPongTimeout), // Short deadline to ensure timely response
)
if err != nil {
return err
}
lastResponse = time.Now()
return nil
})
go func() {
defer ticker.Stop()
for {
<-ticker.C
if time.Since(lastResponse) > timeout {
c.Close()
return
}
}
}()
}

Hello team 👋

I encountered a data race while running go run -race using the go-binance/v2 client in a production-like environment.

The race happens inside the keepAlive() function in websocket.go, where the lastResponse variable is:
• Written inside the SetPingHandler goroutine
• Read inside the ticker goroutine

WARNING: DATA RACE
Read at ... by goroutine A:
  go-binance/v2/futures.keepAlive.func2()
    ↳ websocket.go:111

Previous write at ... by goroutine B:
  go-binance/v2/futures.keepAlive.func1()
    ↳ websocket.go:102
  gorilla/websocket.(*Conn).advanceFrame()
    ↳ conn.go:954
  gorilla/websocket.(*Conn).NextReader()
  gorilla/websocket.(*Conn).ReadMessage()

Goroutine A created at:
  go-binance/v2/futures.keepAlive()
    ↳ websocket.go:107

Goroutine B created at:
  go-binance/v2/futures.WsUserDataServe()
    ↳ websocket_service.go:1198

While it doesn’t crash, this race condition raises concerns during development—especially when running alongside other critical systems—since it triggers race warnings that can undermine confidence in stability.
Thanks for maintaining such a useful library!

@sc0Vu sc0Vu self-assigned this Apr 29, 2025
@sc0Vu
Copy link
Contributor

sc0Vu commented Apr 29, 2025

@Never-Know-N Thanks for reporting this. You're right about the race condition. The lastResponse doesn't look good there. I'll see how to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants