-
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
websocket/gateio: Add request functions for websocket multi-connection [SPOT] #1598
base: master
Are you sure you want to change the base?
Conversation
…larity on purpose. Change connections map to point to candidate to track subscriptions for future dynamic connections holder and drop struct ConnectionDetails.
…rror but websocket frame error or anything really makes the reader routine return and then connection never cycles and the buffer gets filled. * Handle reconnection via an errors.Is check which is simpler and in that scope allow for quick disconnect reconnect without waiting for connection cycle. * Dial now uses code from DialContext but just calls context.Background() * Don't allow reader to return on parse binary response error. Just output error and return a non nil response
…would hang connection reader for eternity.
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'm happy.
2 conversations outstanding where I was asking for changes, but they're trivial.
Address or Ignore :)
… item to nil for systems that do not require rate limiting; add glorious nit
This now requires dependency #1733 for spot trading rate limits |
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'm happy. Only one change which is just a rename. Once ratelimits is merged then I can do a final look
exchanges/stream/stream_match.go
Outdated
// EnsureMatchWithData validates that incoming data matches a request's signature. | ||
// If a match is found, the data is processed; otherwise, it returns an error. | ||
func (m *Match) EnsureMatchWithData(signature any, data []byte) 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.
I think Required
or just Require
versus Ensure
. Ensure sounds like if it fails to do so, it will then do something to make it happen.
Sorry I know both me and gk have tagged you a lot about word choices 😅
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 obviously don't know how to word so it's not surprising.
} | ||
|
||
return json.Unmarshal(inbound.Data, &to) | ||
return json.Unmarshal(inbound.Data, &funnelResult{Result: result}) |
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.
Something for another time is that I think jsonparser may be of use in the
if err := json.Unmarshal(endResponse, &inbound); err != nil {
above this. All you do is check a status and then if failed, the error message. Might be able to save on a double unmarshal. But Sonic the json hedgehog might be faster I can't benchmark that
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.
Yeah good point; I will do some tests on it when I get some time, from what I have seen previously its still lighter to unmarshal everything using sonic rather than even json parser for one item. But mem bad 🤯. Also it might add in overhead from a non error path situation if we look at it from that perspective. We shall see.
removed label because test fix was included in recent commit |
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! Thanks Shazburtino
PR Description
This adds initial functionality for outbound websocket requests on GateIO using multi connection management.
Dependencies:
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