Add user defined callback on http response #335
Conversation
alpaca/rest.go
Outdated
| @@ -30,6 +30,8 @@ type ClientOpts struct { | |||
| RetryDelay time.Duration | |||
| // HTTPClient to be used for each http request. | |||
| HTTPClient *http.Client | |||
| // ResponseCallback is a user defined callback to execute on the raw http.Response | |||
| ResponseCallback func(*http.Response) | |||
There was a problem hiding this comment.
It would be useful to also provide the request.
| ResponseCallback func(*http.Response) | |
| ResponseCallback func(*http.Request, *http.Response) |
There was a problem hiding this comment.
Indeed, I agree it can also be useful, I'll wait for the maintainers feedback and implement this accordingly.
There was a problem hiding this comment.
Yeah, this solution works for me. HTTPCallback works too.
There was a problem hiding this comment.
Will add the request and rename to HTTPCallback than 👍
marketdata/rest.go
Outdated
| @@ -42,6 +42,8 @@ type ClientOpts struct { | |||
| HTTPClient *http.Client | |||
| // Host used to set the http request's host | |||
| RequestHost string | |||
| // ResponseCallback is a user defined callback to execute on the raw http.Response | |||
| ResponseCallback func(*http.Response) | |||
alpaca/rest.go
Outdated
| @@ -114,6 +116,10 @@ func defaultDo(c *Client, req *http.Request) (*http.Response, error) { | |||
| time.Sleep(c.opts.RetryDelay) | |||
| } | |||
|
|
|||
| if c.opts.ResponseCallback != nil { | |||
There was a problem hiding this comment.
The original issue (#292) is explicitly about rate limit. However, in this implementation the callback function is not called when the rate limit is hit (L110), only when either we reach the retry limit or succeed. This means there's no way to delay your next request after hitting 429, making the callback unusable for #292.
There was a problem hiding this comment.
Thanks for looking at this @gnvk!
The main goal of #292 is to expose the rate limit state so we can, for example, make full use of the available request rate for polling (balances, positions, orders, fills) and placing orders, and adjust the polling rate dynamically as needed. This is especially useful when the Alpaca account is accessed from multiple client connections concurrently (e.g. TradingView integration and a locally running trading bot). This PR satisfies that goal.
It looks like it will still call the callback on 429s since L110 only breaks from the for loop but does not return.
There was a problem hiding this comment.
Even so, I'd call the callback when a 429 happens to cover at least one other use case.
There was a problem hiding this comment.
Oh, are you saying we should call the callback the first time we get a 429, rather than waiting until the retry limit is met? If so, I agree.
There was a problem hiding this comment.
I agree, I'll adjust the implementation, thanks for spotting it @gnvk
There was a problem hiding this comment.
I changed the implementation and now I'm invoking the method right after receiving the response instead of deferring the execution
resolves #292