-
Notifications
You must be signed in to change notification settings - Fork 759
Add Binance announcement WebSocket support #750
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
base: master
Are you sure you want to change the base?
Conversation
Allows passing a ConnHandler to wsServeWithConnHandler for custom connection management, enabling active keepalive via ping messages. WsAnnouncementServe now uses this to send periodic pings.
@0x0001 thanks a lot for your contribution, we will review and merge it soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds WebSocket support for Binance announcements, allowing clients to receive real-time announcement notifications from Binance's announcement system. This enables applications to stay updated with important Binance announcements such as delistings, new listings, and other platform updates.
- Adds announcement WebSocket connection functionality with proper authentication
- Implements keepalive mechanisms for WebSocket connections with both ping and pong handlers
- Updates existing WebSocket infrastructure to support custom headers and connection handlers
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
File | Description |
---|---|
v2/annouancement_service.go | Creates announcement parameters with proper authentication for WebSocket connection |
v2/websocket.go | Enhances WebSocket infrastructure with header support and flexible keepalive mechanisms |
v2/websocket_service.go | Implements announcement WebSocket service and updates type assertions to use 'any' |
v2/websocket_service_test.go | Adds comprehensive tests for announcement WebSocket functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
keepAlive(c, WebsocketTimeout) | ||
|
||
// Custom connection handling, useful in active keepalive scenarios | ||
if connHandler != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you wrap keepalive in another connHandler? The connHandler isn't used in other place. How do you think we keep in the old way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The websocket for the Binance announcement requires active ping messages. I want to put passive keepalive and active keepalive into one logic. The active keepalive is used in the WsAnouncementServe
type ConnHandler func(context.Context, *websocket.Conn) | ||
|
||
// WsServeWithConnHandler serves websocket with custom connection handler, useful for custom keepalive | ||
var wsServeWithConnHandler = func(cfg *WsConfig, handler WsHandler, errHandler ErrHandler, connHandler ConnHandler) (doneC, stopC chan struct{}, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs (https://developers.binance.com/docs/cms/general-info), looks like we need to send ping actively. wsServe
seems to keep connection by replying ping from server. Does that work?
handler(e) | ||
} | ||
return wsServeWithConnHandler(cfg, wsHandler, errHandler, keepAliveWithPing(30*time.Second, WebsocketTimeout)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sc0Vu here, send ping actively, in the last arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant wsServe
seems not used here. Should we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wsServe
is used in other websocket services, do we need to change everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, you're right.
v2/websocket.go
Outdated
case <-ctx.Done(): | ||
return | ||
case <-ticker.C: | ||
if err := c.WriteControl(websocket.PingMessage, []byte{}, time.Now().Add(WebsocketPongTimeout)); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you think to add parameter WebsocketPingTimeout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you be more specific? I don't get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, i get it. What do you think about combining ping/pong timeout into WebSocketTimeout?
<-ticker.C | ||
for { | ||
select { | ||
case <-ctx.Done(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we close the connection when context is cancelled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wsServeWithConnHandler
code snippet is as follows
go func() {
defer close(doneC) // 1
if connHandler != nil {
ctx, cancel := context.WithCancel(context.Background())
defer cancel() // 2
go connHandler(ctx, c)
}
silent := false
go func() {
select {
case <-stopC:
silent = true
case <-doneC:
}
c.Close() // 3
}()
for {
_, message, err := c.ReadMessage()
if err != nil {
if !silent {
errHandler(err)
}
return
}
handler(message)
}
}()
// 2 go routine will only cause context cancel when it exits
// 1 If the go routine exits, it will execute close(doneC).
// 3 If doneC is closed, the websocket will be closed
This commit introduces atomic operations for tracking the last response time in websocket keepalive handlers, ensuring thread-safe access to the timestamp. It also adds a new `WebsocketKeepaliveTimeout` configuration variable to control the ping/pong message interval when keepalive is enabled, replacing the previous use of `WebsocketPongTimeout` for this purpose.
v2/websocket_service.go
Outdated
WebsocketTimeout = time.Second * 600 | ||
// WebsocketPongTimeout is an interval for sending a PONG frame in response to PING frame from server | ||
WebsocketPongTimeout = time.Second * 10 | ||
// WebsocketKeepaliveTimeout is an interval for sending ping/pong messages if WebsocketKeepalive is enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used WebsocketPongTimeout
in other ws client, like delivery. IMHO, it's much easier to add WebsocketPingTimeout
here. How do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, Although I would have preferred to standardize on one variable, because when using it externally, you don't need to know whether the current websocket method is maintained by ping or pong, which is more cohesive. However, since the original variable has already been exported, changing the name of the variable directly will cause problems for users of the library.
The timeout variable names are now consistent with their actual usage: - WebsocketPingTimeout for PING frame deadlines - WebsocketPongTimeout for PONG frame deadlines This change affects both websocket.go and websocket_service.go files.
3d41c9e
to
86649a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
WebsocketTimeout = time.Second * 600 | ||
// WebsocketPongTimeout is an interval for sending a PONG frame in response to PING frame from server | ||
WebsocketPongTimeout = time.Second * 10 | ||
/// WebsocketPingTimeout is an interval for sending a PING frame in response to PONG frame from server |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect comment format and description. Should use standard Go comment format with '//' and correct description: 'WebsocketPingTimeout is an interval for waiting for a PONG response after sending a PING frame'
/// WebsocketPingTimeout is an interval for sending a PING frame in response to PONG frame from server | |
// WebsocketPingTimeout is an interval for waiting for a PONG response after sending a PING frame |
Copilot uses AI. Check for mistakes.
if event.Topic != "com_announcement_en" { | ||
errHandler(errors.New("topic is not com_announcement_en: " + event.Topic)) | ||
return | ||
} |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded topic string should be extracted as a constant to avoid duplication and improve maintainability. The same string 'com_announcement_en' is used in announcement_service.go line 45.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sc0Vu This only repeats twice, and probably won't repeat more than that. I don't think it's necessary to define it as a constant. What's your take?
return WsAnnouncementParam{}, err | ||
} | ||
r := make([]byte, 16) | ||
rand.Read(r) |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error from rand.Read should be checked. Although rand.Read from crypto/rand is documented to never return an error in practice, it's a security best practice to handle potential errors when generating cryptographic random values.
rand.Read(r) | |
if _, err := rand.Read(r); err != nil { | |
return WsAnnouncementParam{}, fmt.Errorf("failed to generate random bytes: %w", err) | |
} |
Copilot uses AI. Check for mistakes.
How to use