Safer HTTP client initialization and usage. #458
Conversation
REST and Slack clients are now only initialized if they haven't already, plus are initialized with a lock on the configuration to avoid contention cases. One notably improvement is that this saves us from validating auth every time the REST client is referenced.
Hopefully this provides more visibiility into cases where a legitimate error interferes with client creation.
| c.mu.Lock() | ||
| defer c.mu.Unlock() |
There was a problem hiding this comment.
haven't used mutexes before in Go, but pleasantly surprised how simple this looks! that's all it takes to handle "a bunch of different callers ask for a client at the same time, and we only want to make one, and have all my callers wait until i make that and give the single one to everyone" ? :o cooool
There was a problem hiding this comment.
Thanks @dobs !! I think this will be a great improvement to user experience.
|
Amazing ! Thanks I will test this because I had to rollback to 1.x. |
|
@NargiT Great! Let us know whether it resolves the issue you encountered. One thing to call out is that these segmentation faults could be a result of some other underlying error (e.g. rate limiting), but hopefully this change will at least make those errors more actionable. |
This PR has two major components (corresponding to the two commits):
By exposing new errors this change may break some expectations around what errors might be returned by the provider, but hopefully in a way that's useful (i.e. before it would segfault, now it'll hopefully return something more informative).
The one gotcha would be if client re-initialization was intentional, but hopefully that isn't the case. And one optimization might be to explicitly initialize the client at config time, after which we don't need error handling around
Client()calls.This PR likely addresses the segfaults mentioned in issues: