feat(flipt): add WithHTTPClient option for custom HTTP client configuration #784
Conversation
…ration Add WithHTTPClient option to both provider and transport layers to allow users to configure custom HTTP clients. This enables use cases like custom timeouts, proxies, or other HTTP client customizations. The option defaults to http.DefaultClient when not specified. Signed-off-by: Roman Dmytrenko <[email protected]>
Summary of ChangesHello @erka, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Flipt provider by introducing the capability for users to supply a custom HTTP client. This feature provides greater flexibility for integrating the provider into diverse network environments, supporting advanced configurations like custom timeouts or proxy settings. The change ensures that if no custom client is provided, the standard Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the WithHTTPClient option to the Flipt provider and transport layers, enabling users to configure custom HTTP clients for use cases like custom timeouts and proxies. The option defaults to http.DefaultClient when not specified. The changes include modifications to go.sum, provider.go, provider_test.go, service.go, and service_test.go to incorporate the new option and its associated functionality. I have provided review comments to address a potential issue with concurrent access to the HTTP client in the transport service.
| once sync.Once | ||
| tokenProvider sdk.ClientTokenProvider | ||
| grpcDialOptions []grpc.DialOption | ||
| httpClient *http.Client |
There was a problem hiding this comment.
The httpClient field in the Service struct is not protected by a mutex. If multiple goroutines access the Service concurrently, they could potentially modify the httpClient simultaneously, leading to a race condition. To prevent this, consider adding a mutex to protect access to the httpClient field.
type Service struct {
// ... other fields
httpClient *http.Client
httpClientMutex sync.RWMutex // Add this mutex
}Then, use the mutex to protect access to the httpClient in the WithHTTPClient option and the instance method.
| func WithHTTPClient(client *http.Client) Option { | ||
| return func(s *Service) { | ||
| s.httpClient = client |
There was a problem hiding this comment.
To ensure thread-safe access to the httpClient, acquire a write lock on the httpClientMutex before modifying the httpClient and release it afterwards.
Consider the following code suggestion to address this issue.
func WithHTTPClient(client *http.Client) Option {
return func(s *Service) {
// Acquire a write lock before modifying the httpClient.
// Release the lock after the modification is complete.
// This ensures that only one goroutine can modify the httpClient at a time.
// Prevents race conditions.
s.httpClientMutex.Lock()
s.httpClient = client
s.httpClientMutex.Unlock()
}
}| } | ||
|
|
||
| hclient := sdk.New(sdkhttp.NewTransport(s.address), opts...) | ||
| hclient := sdk.New(sdkhttp.NewTransport(s.address, sdkhttp.WithHTTPClient(s.httpClient)), opts...) |
There was a problem hiding this comment.
To ensure thread-safe access to the httpClient, acquire a read lock on the httpClientMutex before accessing the httpClient and release it afterwards.
Consider the following code suggestion to address this issue.
// Acquire a read lock before accessing the httpClient.
// Release the lock after the access is complete.
// This allows multiple goroutines to read the httpClient concurrently,
// but prevents them from modifying it while it's being read.
s.httpClientMutex.RLock()
hclient := sdk.New(sdkhttp.NewTransport(s.address, sdkhttp.WithHTTPClient(s.httpClient)), opts...)
s.httpClientMutex.RUnlock()
if u.Scheme == "https" || u.Scheme == "http" {2 participants
This PR
Add WithHTTPClient option to both provider and transport layers to allow
users to configure custom HTTP clients. This enables use cases like custom
timeouts, proxies, or other HTTP client customizations. The option defaults
to http.DefaultClient when not specified.