Skip to content
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

client: support to set timeout in http client #7847

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions client/http/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,9 @@
httpsScheme = "https"
networkErrorStatus = "network error"

defaultMembersInfoUpdateInterval = time.Minute
defaultTimeout = 30 * time.Second
// The value of defaultTimeout is the maximum value.
// If you want to shorten timeout, declare it when creating the client
defaultTimeout = 300 * time.Second
)

// respHandleFunc is the function to handle the HTTP response.
Expand Down Expand Up @@ -77,7 +78,7 @@
func (ci *clientInner) init(sd pd.ServiceDiscovery) {
// Init the HTTP client if it's not configured.
if ci.cli == nil {
ci.cli = &http.Client{Timeout: defaultTimeout}
ci.cli = &http.Client{}
if ci.tlsConf != nil {
transport := http.DefaultTransport.(*http.Transport).Clone()
transport.TLSClientConfig = ci.tlsConf
Expand Down Expand Up @@ -172,6 +173,7 @@
body = reqInfo.body
res = reqInfo.res
respHandler = reqInfo.respHandler
cancel context.CancelFunc
)
logFields := []zap.Field{
zap.String("source", source),
Expand All @@ -181,6 +183,8 @@
zap.String("caller-id", callerID),
}
log.Debug("[pd] request the http url", logFields...)
ctx, cancel = context.WithTimeout(ctx, reqInfo.timeout)
Copy link
Contributor

@niubell niubell Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set a timeout for each request may not be a good idea. How about just supporting only one way to set timeout: init timeout in client with WithTimeout function, and timeout mechanism also through Timeout in http.Client/http.Transport?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. My initial idea was to create as few http.clients as possible, but I've found that this is not a good way to modify it.

defer cancel()
req, err := http.NewRequestWithContext(ctx, method, url, bytes.NewBuffer(body))
if err != nil {
log.Error("[pd] create http request failed", append(logFields, zap.Error(err))...)
Expand Down Expand Up @@ -244,6 +248,7 @@
callerID string
respHandler respHandleFunc
bo *retry.Backoffer
timeout time.Duration
}

// ClientOption configures the HTTP client.
Expand All @@ -256,6 +261,13 @@
}
}

// WithTimeout configures the client with the given timeout config.
func WithTimeout(dur time.Duration) ClientOption {
return func(c *client) {
c.timeout = dur

Check warning on line 267 in client/http/client.go

View check run for this annotation

Codecov / codecov/patch

client/http/client.go#L267

Added line #L267 was not covered by tests
}
}

// WithTLSConfig configures the client with the given TLS config.
// This option won't work if the client is configured with WithHTTPClient.
func WithTLSConfig(tlsConf *tls.Config) ClientOption {
Expand Down Expand Up @@ -342,6 +354,13 @@
return &newClient
}

// WithTimeout sets and returns a new client with the given timeout config.
func (c *client) WithTimeout(dur time.Duration) Client {
newClient := *c
newClient.timeout = dur
return &newClient
}

// Header key definition constants.
const (
pdAllowFollowerHandleKey = "PD-Allow-Follower-Handle"
Expand All @@ -362,7 +381,8 @@
return c.inner.requestWithRetry(ctx, reqInfo.
WithCallerID(c.callerID).
WithRespHandler(c.respHandler).
WithBackoffer(c.bo),
WithBackoffer(c.bo).
WithTimeout(c.timeout),
headerOpts...)
}

Expand Down
3 changes: 3 additions & 0 deletions client/http/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"net/http"
"strconv"
"strings"
"time"

"github.com/pingcap/errors"
"github.com/pingcap/kvproto/pkg/metapb"
Expand Down Expand Up @@ -105,6 +106,8 @@ type Client interface {
WithRespHandler(func(resp *http.Response, res any) error) Client
// WithBackoffer sets and returns a new client with the given backoffer.
WithBackoffer(*retry.Backoffer) Client
// WithTimeout sets and returns a new client with the timeout config.
WithTimeout(time.Duration) Client
// Close gracefully closes the HTTP client.
Close()
}
Expand Down
14 changes: 13 additions & 1 deletion client/http/request_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package http

import (
"fmt"
"time"

"github.com/tikv/pd/client/retry"
)
Expand Down Expand Up @@ -88,11 +89,14 @@ type requestInfo struct {
res any
respHandler respHandleFunc
bo *retry.Backoffer
timeout time.Duration
}

// newRequestInfo creates a new request info.
func newRequestInfo() *requestInfo {
return &requestInfo{}
return &requestInfo{
timeout: defaultTimeout,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about setting a default timeout for the client and passing it through? Leave therequestInfo always empty to customize.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that it is better to set default timeout for http.Client?
I can have a try, but we should replace innter client to do it.
The reason why I made this change is because:

  1. I just want to initialize an http.Client
  2. The timeout value of http.Client used in TiDB is 0
  3. The timeout value of http.Client used in client-go is 30s

}
}

// WithCallerID sets the caller ID of the request.
Expand Down Expand Up @@ -143,6 +147,14 @@ func (ri *requestInfo) WithBackoffer(bo *retry.Backoffer) *requestInfo {
return ri
}

// WithTimeout sets the timeout of the request.
func (ri *requestInfo) WithTimeout(dur time.Duration) *requestInfo {
if dur > 0 {
ri.timeout = dur
}
return ri
}

func (ri *requestInfo) getURL(addr string) string {
return fmt.Sprintf("%s%s", addr, ri.uri)
}
6 changes: 6 additions & 0 deletions server/api/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ package api

import (
"net/http"
"time"

"github.com/pingcap/failpoint"
"github.com/tikv/pd/pkg/versioninfo"
"github.com/unrolled/render"
)
Expand Down Expand Up @@ -47,5 +49,9 @@ func (h *versionHandler) GetVersion(w http.ResponseWriter, r *http.Request) {
version := &version{
Version: versioninfo.PDReleaseVersion,
}
failpoint.Inject("slowGetVersion", func(val failpoint.Value) {
//nolint:durationcheck
time.Sleep(time.Millisecond * time.Duration(val.(int)))
})
h.rd.JSON(w, http.StatusOK, version)
}
33 changes: 32 additions & 1 deletion tests/integrations/client/http_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package client_test

import (
"context"
"fmt"
"math"
"net/http"
"net/url"
Expand All @@ -25,6 +26,7 @@ import (
"time"

"github.com/pingcap/errors"
"github.com/pingcap/failpoint"
"github.com/prometheus/client_golang/prometheus"
dto "github.com/prometheus/client_model/go"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -118,7 +120,7 @@ func (suite *httpClientTestSuite) TearDownSuite() {
}

// RunTestInTwoModes is to run test in two modes.
func (suite *httpClientTestSuite) RunTestInTwoModes(test func(mode mode, client pd.Client)) {
func (suite *httpClientTestSuite) RunTestInTwoModes(test func(mode mode, client pd.Client), opts ...pd.ClientOption) {
// Run test with specific service discovery.
cli := setupCli(suite.Require(), suite.env[specificServiceDiscovery].ctx, suite.env[specificServiceDiscovery].endpoints)
sd := cli.GetServiceDiscovery()
Expand Down Expand Up @@ -680,6 +682,35 @@ func (suite *httpClientTestSuite) checkWithBackoffer(mode mode, client pd.Client
re.Nil(rule)
}

func (suite *httpClientTestSuite) TestWithTimeout() {
suite.RunTestInTwoModes(suite.checkWithTimeout, pd.WithTimeout(1*time.Second))
}

func (suite *httpClientTestSuite) checkWithTimeout(mode mode, client pd.Client) {
re := suite.Require()
env := suite.env[mode]

re.NoError(failpoint.Enable("github.com/tikv/pd/server/api/slowGetVersion", fmt.Sprintf("return(%d)", 700)))
ctx, cancel := context.WithTimeout(env.ctx, 500*time.Millisecond)
_, err := client.GetPDVersion(ctx)
re.ErrorIs(err, context.DeadlineExceeded)
cancel()

newClient := client.WithTimeout(500 * time.Millisecond)
_, err = newClient.GetPDVersion(env.ctx)
re.ErrorIs(err, context.DeadlineExceeded)

version, err := client.GetPDVersion(env.ctx)
re.NoError(err)
re.Equal("None", version)
re.NoError(failpoint.Disable("github.com/tikv/pd/server/api/slowGetVersion"))

re.NoError(failpoint.Enable("github.com/tikv/pd/server/api/slowGetVersion", fmt.Sprintf("return(%d)", 1200)))
_, err = client.GetPDVersion(ctx)
re.ErrorIs(err, context.DeadlineExceeded)
re.NoError(failpoint.Disable("github.com/tikv/pd/server/api/slowGetVersion"))
}

func (suite *httpClientTestSuite) TestRedirectWithMetrics() {
re := suite.Require()
env := suite.env[defaultServiceDiscovery]
Expand Down
Loading