-
Notifications
You must be signed in to change notification settings - Fork 820
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
common/gateio/stream: add thread-safe counter and overide default GenerateMessageID with connection specific implementation #1615
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1615 +/- ##
==========================================
+ Coverage 36.37% 36.38% +0.01%
==========================================
Files 422 422
Lines 183113 183135 +22
==========================================
+ Hits 66602 66636 +34
+ Misses 108466 108452 -14
- Partials 8045 8047 +2
|
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.
Only one request, this is fine 🐶
// IncrementAndGet returns the next count after incrementing. | ||
func (c *Counter) IncrementAndGet() int64 { | ||
newID := atomic.AddInt64(&c.n, 1) | ||
// Handle overflow by resetting the counter to 1 if it becomes negative |
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.
Had thoughts of suggesting uint64 instead, but there are many unknowns and I doubt anyone is going to subscribe to WIF more 9,223,372,036,854,775,807 times on GateIO in a single session
Co-authored-by: Scott <[email protected]>
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.
tACK!
🌞
[DEBUG] | WEBSOCKET | 16/08/2024 13:37:42 | GateIO websocket connection: sending message [{"time":1723779462,"id":1,"channel":"spot.tickers","event":"subscribe","payload":["BTC_USDT"]}]
[DEBUG] | WEBSOCKET | 16/08/2024 13:37:42 | GateIO websocket connection: message received: {"time":1723779462,"time_ms":1723779462323,"id":1,"conn_id":"1b73153490cc89f8","trace_id":"daa79b26d14a54b172e11012230c1c78","channel":"spot.tickers","event":"subscribe","payload":["BTC_USDT"],"result":{"status":"success"},"requestId":"daa79b26d14a54b172e11012230c1c78"}
[DEBUG] | WEBSOCKET | 16/08/2024 13:37:42 | GateIO websocket connection: sending message [{"time":1723779462,"id":2,"channel":"spot.candlesticks","event":"subscribe","payload":["5m","BTC_USDT"]}]
[DEBUG] | WEBSOCKET | 16/08/2024 13:37:42 | GateIO websocket connection: message received: {"time":1723779462,"time_ms":1723779462670,"id":2,"conn_id":"1b73153490cc89f8","trace_id":"daa79b26d14a54b172e11012230c1c78","channel":"spot.candlesticks","event":"subscribe","payload":["5m","BTC_USDT"],"result":{"status":"success"},"requestId":"daa79b26d14a54b172e11012230c1c78"}
[DEBUG] | WEBSOCKET | 16/08/2024 13:37:42 | GateIO websocket connection: sending message [{"time":1723779462,"id":3,"channel":"spot.book_ticker","event":"subscribe","payload":["BTC_USDT"]}]
[DEBUG] | WEBSOCKET | 16/08/2024 13:37:43 | GateIO websocket connection: message received: {"time":1723779463,"time_ms":1723779463017,"id":3,"conn_id":"1b73153490cc89f8","trace_id":"daa79b26d14a54b172e11012230c1c78","channel":"spot.book_ticker","event":"subscribe","payload":["BTC_USDT"],"result":{"status":"success"},"requestId":"daa79b26d14a54b172e11012230c1c78"}
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.
Rather than deviating from the websocket GenerateMessageID helper and rolling out exchange specific counters, I'd opt for the ability to override the default behavior so it still remains unified but configurable
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.
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.
Just realised after tACK'ing that this requires test coverage and then should be good to go
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.
Changes ACK, thanks!
PR Description
atomicitycollision resistance, this update adds a basic counter for routines that require low cost message matching signatures.GenerateMessageID
default
GenerateMessageID is quite highFixes # (issue)
Type of change
Please delete options that are not relevant and add an
x
in[]
as item is complete.How has this been tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.
Checklist