From 6555671eb344feb3b392c7146fc90732ffa610d9 Mon Sep 17 00:00:00 2001 From: Taichi Sasaki Date: Tue, 6 May 2025 10:38:47 +0900 Subject: [PATCH 1/2] Make it possible to set timeout to health.NewPinger() --- health/pinger.go | 27 ++++++++++++++++++++------- health/pinger_test.go | 38 +++++++++++++++++++++++++++++++------- 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/health/pinger.go b/health/pinger.go index 3ed57ff6..4955fb63 100644 --- a/health/pinger.go +++ b/health/pinger.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/url" + "time" ) type ( @@ -21,13 +22,15 @@ type ( Option func(o *options) client struct { - name string - req *http.Request + name string + req *http.Request + timeout time.Duration } options struct { - scheme string - path string + scheme string + path string + timeout time.Duration } ) @@ -45,8 +48,9 @@ func NewPinger(name, addr string, opts ...Option) Pinger { panic(err) } return &client{ - name: name, - req: req, + name: name, + req: req, + timeout: options.timeout, } } @@ -55,7 +59,8 @@ func (c *client) Name() string { } func (c *client) Ping(ctx context.Context) error { - resp, err := http.DefaultClient.Do(c.req.WithContext(ctx)) + httpClient := &http.Client{Timeout: c.timeout} + resp, err := httpClient.Do(c.req.WithContext(ctx)) if err != nil { return fmt.Errorf("failed to make health check request to %q: %v", c.name, err) } @@ -81,3 +86,11 @@ func WithPath(path string) Option { o.path = path } } + +// WithTimeout sets the timeout used to ping the service. +// Default is no timeout. +func WithTimeout(timeout time.Duration) Option { + return func(o *options) { + o.timeout = timeout + } +} diff --git a/health/pinger_test.go b/health/pinger_test.go index b8c7303c..9c25d0d4 100644 --- a/health/pinger_test.go +++ b/health/pinger_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "net/url" "testing" + "time" ) func TestPing(t *testing.T) { @@ -58,18 +59,38 @@ func TestPing(t *testing.T) { } }) } + t.Run("timeout", func(t *testing.T) { + handler := func(w http.ResponseWriter, r *http.Request) { + time.Sleep(2 * time.Second) + w.WriteHeader(http.StatusOK) + } + svr := httptest.NewServer(http.HandlerFunc(handler)) + defer svr.Close() + u, _ := url.Parse(svr.URL) + pinger := NewPinger("dependency", u.Host, WithTimeout(time.Second)) + if pinger.Name() != "dependency" { + t.Errorf("got name: %s, expected dependency", pinger.Name()) + } + err := pinger.Ping(context.Background()) + expected := fmt.Errorf(`failed to make health check request to "dependency": Get "%s/livez": context deadline exceeded (Client.Timeout exceeded while awaiting headers)`, svr.URL) + if err != nil && err.Error() != expected.Error() { + t.Errorf("got: %v, expected: %v", err, expected) + } + }) } func TestOptions(t *testing.T) { cases := []struct { - name string - option Option - expectedScheme string - expectedPath string + name string + option Option + expectedScheme string + expectedPath string + expectedTimeout time.Duration }{ - {"default", nil, "http", "/livez"}, - {"scheme", WithScheme("https"), "https", "/livez"}, - {"path", WithPath("/healthcheck"), "http", "/healthcheck"}, + {"default", nil, "http", "/livez", 0}, + {"scheme", WithScheme("https"), "https", "/livez", 0}, + {"path", WithPath("/healthcheck"), "http", "/healthcheck", 0}, + {"timeout", WithTimeout(10 * time.Second), "http", "/livez", 10 * time.Second}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -88,6 +109,9 @@ func TestOptions(t *testing.T) { if pinger.req.URL.Path != c.expectedPath { t.Errorf("got path: %s, expected %s", pinger.req.URL.Path, c.expectedPath) } + if pinger.timeout != c.expectedTimeout { + t.Errorf("got timeout: %s, expected %s", pinger.timeout, c.expectedTimeout) + } }) } } From 379eb114a643aa859c5b4ef970183879d4347649 Mon Sep 17 00:00:00 2001 From: Taichi Sasaki Date: Tue, 6 May 2025 11:13:12 +0900 Subject: [PATCH 2/2] Reuse http.Client for health.NewPinger() --- health/pinger.go | 15 +++++++-------- health/pinger_test.go | 4 ++-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/health/pinger.go b/health/pinger.go index 4955fb63..86825995 100644 --- a/health/pinger.go +++ b/health/pinger.go @@ -22,9 +22,9 @@ type ( Option func(o *options) client struct { - name string - req *http.Request - timeout time.Duration + name string + req *http.Request + httpClient *http.Client } options struct { @@ -48,9 +48,9 @@ func NewPinger(name, addr string, opts ...Option) Pinger { panic(err) } return &client{ - name: name, - req: req, - timeout: options.timeout, + name: name, + req: req, + httpClient: &http.Client{Timeout: options.timeout}, } } @@ -59,8 +59,7 @@ func (c *client) Name() string { } func (c *client) Ping(ctx context.Context) error { - httpClient := &http.Client{Timeout: c.timeout} - resp, err := httpClient.Do(c.req.WithContext(ctx)) + resp, err := c.httpClient.Do(c.req.WithContext(ctx)) if err != nil { return fmt.Errorf("failed to make health check request to %q: %v", c.name, err) } diff --git a/health/pinger_test.go b/health/pinger_test.go index 9c25d0d4..8302a4cc 100644 --- a/health/pinger_test.go +++ b/health/pinger_test.go @@ -109,8 +109,8 @@ func TestOptions(t *testing.T) { if pinger.req.URL.Path != c.expectedPath { t.Errorf("got path: %s, expected %s", pinger.req.URL.Path, c.expectedPath) } - if pinger.timeout != c.expectedTimeout { - t.Errorf("got timeout: %s, expected %s", pinger.timeout, c.expectedTimeout) + if pinger.httpClient.Timeout != c.expectedTimeout { + t.Errorf("got timeout: %s, expected %s", pinger.httpClient.Timeout, c.expectedTimeout) } }) }