feat(client): add option to retry only idempotent requests (8260b221) · Commits · GitLab.org / API / client-go

client_options.go

+16 −0

Original line number	Diff line number	Diff line
		@@ -143,6 +143,22 @@ func WithoutRetries() ClientOptionFunc {
		}
		}

		// WithOnlyIdempotentRetries restricts automatic retries to requests using an
		// idempotent HTTP method: GET, HEAD, OPTIONS, TRACE, PUT, DELETE or QUERY
		// (RFC 9110 §9.2.2, plus QUERY from RFC 10008). Notably, a 5xx response to a
		// POST is then not retried, as the server may have already applied its side
		// effect, e.g. creating a duplicate comment. A 429 response is still retried
		// for any method, as a rate-limited request is rejected before it is
		// processed.
		//
		// This is expected to become the default behavior in the next major release.
		func WithOnlyIdempotentRetries() ClientOptionFunc {
		return func(c *Client) error {
		c.onlyIdempotentRetries = true
		return nil
		}
		}

		// WithRequestOptions can be used to configure default request options applied to every request.
		func WithRequestOptions(options ...RequestOptionFunc) ClientOptionFunc {
		return func(c *Client) error {

gitlab.go

+52 −10

Original line number	Diff line number	Diff line
		@@ -124,6 +124,10 @@ type Client struct {
		// disableRetries is used to disable the default retry logic.
		disableRetries bool

		// onlyIdempotentRetries restricts automatic retries to safe or idempotent
		// HTTP methods. Set via WithOnlyIdempotentRetries.
		onlyIdempotentRetries bool

		// configureLimiterOnce is used to make sure the limiter is configured exactly
		// once and block all other calls until the initial (one) call is done.
		configureLimiterOnce sync.Once
		@@ -689,15 +693,11 @@ func (c Client) retryHTTPCheck(ctx context.Context, resp http.Response, err er
		}

		if err != nil {
		// We should be able to retry requests with assumed idempotent HTTP methods.
		// In a future iteration we might want to annotate requests that are idempotent
		// to further improve this logic here.
		if resp != nil && resp.Request != nil {
		switch resp.Request.Method {
		case http.MethodConnect, http.MethodOptions, http.MethodTrace, http.MethodHead, http.MethodGet:
		// Retry a method that is safe to repeat: doing so cannot duplicate a
		// side effect, even if the error occurred after the request was written.
		if resp != nil && resp.Request != nil && c.isMethodRetryable(resp.Request.Method) {
		return true, nil
		}
		}

		// Only retry errors that we know happened before writing to the wire
		var urlErr *url.Error
		@@ -765,14 +765,56 @@ func (c Client) retryHTTPCheck(ctx context.Context, resp http.Response, err er
		// invalid response codes as well, like 0 and 999.
		// Status code 0 is especially important for AWS ALB:
		// https://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-troubleshooting.html#response-code-000
		//
		// With WithOnlyIdempotentRetries, only do so for an idempotent method,
		// since the server may have already acted on the request. A nil Request
		// keeps the prior behavior.
		if resp.StatusCode == 0 \|\| (resp.StatusCode >= 500 && resp.StatusCode != http.StatusNotImplemented) {
		// return true, fmt.Errorf("unexpected HTTP status %s", resp.Status)
		if !c.onlyIdempotentRetries \|\| resp.Request == nil \|\| isMethodIdempotent(resp.Request.Method) {
		return true, nil
		}
		}

		return false, nil
		}

		// isMethodRetryable reports whether a request using the given method may be
		// retried after a network error.
		func (c *Client) isMethodRetryable(method string) bool {
		if c.onlyIdempotentRetries {
		return isMethodIdempotent(method)
		}

		// We should be able to retry requests with assumed idempotent HTTP methods.
		// In a future iteration we might want to annotate requests that are idempotent
		// to further improve this logic here.
		switch method {
		case http.MethodConnect, http.MethodOptions, http.MethodTrace, http.MethodHead, http.MethodGet:
		return true
		default:
		return false
		}
		}

		// isMethodIdempotent reports whether the given method is idempotent (RFC 9110
		// §9.2.2, plus QUERY from RFC 10008), making it safe to retry automatically;
		// retrying a non-idempotent method such as POST could duplicate a side effect
		// the server already applied.
		func isMethodIdempotent(method string) bool {
		switch method {
		case http.MethodGet,
		http.MethodHead,
		http.MethodOptions,
		http.MethodTrace,
		http.MethodPut,
		http.MethodDelete,
		"QUERY": // No net/http constant for QUERY yet.
		return true
		default:
		return false
		}
		}

		// retryHTTPBackoff provides a generic callback for Client.Backoff which
		// will pass through all calls based on the status code of the response.
		func (c Client) retryHTTPBackoff(min, max time.Duration, attemptNum int, resp http.Response) time.Duration {

gitlab_test.go

+122 −23

Original line number	Diff line number	Diff line
		@@ -56,15 +56,15 @@ var (
		// setup sets up a test HTTP server along with a gitlab.Client that is
		// configured to talk to that test server. Tests should register handlers on
		// mux which provide mock responses for the API method being tested.
		func setup(t testing.T) (http.ServeMux, *Client) {
		func setup(t testing.T, options ...ClientOptionFunc) (http.ServeMux, *Client) {
		// Setup on subpath, but pass in an empty subpath.
		return setupOnSubpath(t, "")
		return setupOnSubpath(t, "", options...)
		}

		// setupOnSubpath sets up a test HTTP server along with a gitlab.Client that is
		// configured to talk to that test server, but it sets up the client to use a baseURL
		// configured on a subpath instead of the root of the domain.
		func setupOnSubpath(t testing.T, path string) (http.ServeMux, *Client) {
		func setupOnSubpath(t testing.T, path string, options ...ClientOptionFunc) (http.ServeMux, *Client) {
		// mux is the HTTP request multiplexer used with the test server.
		mux := http.NewServeMux()

		@@ -73,13 +73,13 @@ func setupOnSubpath(t testing.T, path string) (http.ServeMux, *Client) {
		t.Cleanup(server.Close)

		// client is the GitLab client being tested.
		client, err := NewClient("",
		client, err := NewClient("", append([]ClientOptionFunc{
		WithBaseURL(server.URL + path),
		// Disable backoff to speed up tests that expect errors.
		WithCustomBackoff(func(_, _ time.Duration, _ int, _ *http.Response) time.Duration {
		return 0
		}),
		)
		}, options...)...)
		require.NoError(t, err)

		return mux, client
		@@ -1181,34 +1181,133 @@ func TestClient_DefaultRetryPolicy_RetryOnIdempotentRequests_ByMethod(t *testing

		tests := []struct {
		method string
		// expectedRetry is the default; expectedRetryOnlyIdempotent applies
		// with the WithOnlyIdempotentRetries option.
		expectedRetry bool
		expectedRetryOnlyIdempotent bool
		}{
		{
		method: http.MethodGet,
		expectedRetry: true,
		},
		{
		method: http.MethodPost,
		expectedRetry: false,
		},
		{method: http.MethodGet, expectedRetry: true, expectedRetryOnlyIdempotent: true},
		{method: http.MethodHead, expectedRetry: true, expectedRetryOnlyIdempotent: true},
		{method: http.MethodOptions, expectedRetry: true, expectedRetryOnlyIdempotent: true},
		{method: http.MethodTrace, expectedRetry: true, expectedRetryOnlyIdempotent: true},
		{method: http.MethodConnect, expectedRetry: true, expectedRetryOnlyIdempotent: false},
		{method: http.MethodPut, expectedRetry: false, expectedRetryOnlyIdempotent: true},
		{method: http.MethodDelete, expectedRetry: false, expectedRetryOnlyIdempotent: true},
		{method: "QUERY", expectedRetry: false, expectedRetryOnlyIdempotent: true},
		{method: http.MethodPost, expectedRetry: false, expectedRetryOnlyIdempotent: false},
		{method: http.MethodPatch, expectedRetry: false, expectedRetryOnlyIdempotent: false},
		}

		for _, tt := range tests {
		t.Run(tt.method, func(t *testing.T) {
		for name, opts := range map[string][]ClientOptionFunc{
		"default": {WithCustomRetryMax(1)},
		"only idempotent": {WithCustomRetryMax(1), WithOnlyIdempotentRetries()},
		} {
		t.Run(tt.method+" "+name, func(t *testing.T) {
		t.Parallel()

		// GIVEN
		client, err := NewClient(
		"",
		WithCustomRetryMax(1),
		)
		client, err := NewClient("", opts...)
		require.NoError(t, err)

		expectedRetry := tt.expectedRetry
		if name == "only idempotent" {
		expectedRetry = tt.expectedRetryOnlyIdempotent
		}

		// WHEN
		retry, err := client.retryHTTPCheck(t.Context(), &http.Response{Request: &http.Request{Method: tt.method}}, errors.New("dummy"))

		// THEN
		assert.Equal(t, tt.expectedRetry, retry)
		assert.Equal(t, expectedRetry, retry)
		assert.NoError(t, err)
		})
		}
		}
		}

		// TestClient_DefaultRetryPolicy_RetryOn5xxStatus_ByMethod verifies a 5xx is
		// retried for any method by default, while with WithOnlyIdempotentRetries it
		// is only retried for safe or idempotent methods, so a POST is not duplicated.
		func TestClient_DefaultRetryPolicy_RetryOn5xxStatus_ByMethod(t *testing.T) {
		t.Parallel()

		tests := []struct {
		method string
		// expectedRetryOnlyIdempotent applies with the
		// WithOnlyIdempotentRetries option; the default is always to retry.
		expectedRetryOnlyIdempotent bool
		}{
		{method: http.MethodGet, expectedRetryOnlyIdempotent: true},
		{method: http.MethodHead, expectedRetryOnlyIdempotent: true},
		{method: http.MethodOptions, expectedRetryOnlyIdempotent: true},
		{method: http.MethodTrace, expectedRetryOnlyIdempotent: true},
		{method: http.MethodPut, expectedRetryOnlyIdempotent: true},
		{method: http.MethodDelete, expectedRetryOnlyIdempotent: true},
		{method: "QUERY", expectedRetryOnlyIdempotent: true},
		{method: http.MethodPost, expectedRetryOnlyIdempotent: false},
		{method: http.MethodPatch, expectedRetryOnlyIdempotent: false},
		{method: http.MethodConnect, expectedRetryOnlyIdempotent: false},
		}

		for _, tt := range tests {
		for name, opts := range map[string][]ClientOptionFunc{
		"default": {WithCustomRetryMax(1)},
		"only idempotent": {WithCustomRetryMax(1), WithOnlyIdempotentRetries()},
		} {
		t.Run(tt.method+" "+name, func(t *testing.T) {
		t.Parallel()

		// GIVEN
		client, err := NewClient("", opts...)
		require.NoError(t, err)

		expectedRetry := true
		if name == "only idempotent" {
		expectedRetry = tt.expectedRetryOnlyIdempotent
		}

		resp := &http.Response{
		StatusCode: http.StatusInternalServerError,
		Request: &http.Request{Method: tt.method},
		}

		// WHEN
		retry, err := client.retryHTTPCheck(t.Context(), resp, nil)

		// THEN
		assert.Equal(t, expectedRetry, retry)
		assert.NoError(t, err)
		})
		}
		}
		}

		// TestClient_DefaultRetryPolicy_RetryOnTooManyRequests_AnyMethod verifies a
		// 429 is retried for any method, even with WithOnlyIdempotentRetries; a
		// rate-limited request is rejected before the server processes it, so a retry
		// cannot duplicate a side effect.
		func TestClient_DefaultRetryPolicy_RetryOnTooManyRequests_AnyMethod(t *testing.T) {
		t.Parallel()

		for _, method := range []string{http.MethodGet, http.MethodPost, http.MethodPatch} {
		t.Run(method, func(t *testing.T) {
		t.Parallel()

		// GIVEN
		client, err := NewClient("", WithCustomRetryMax(1), WithOnlyIdempotentRetries())
		require.NoError(t, err)

		resp := &http.Response{
		StatusCode: http.StatusTooManyRequests,
		Request: &http.Request{Method: method},
		}

		// WHEN
		retry, err := client.retryHTTPCheck(t.Context(), resp, nil)

		// THEN
		assert.True(t, retry)
		assert.NoError(t, err)
		})
		}

project_markdown_uploads_test.go

+19 −0

Original line number	Diff line number	Diff line
		@@ -84,6 +84,25 @@ func TestMarkdownUploads_UploadProjectMarkdown_Retry(t *testing.T) {
		assert.Equal(t, want, upload)
		}

		// TestMarkdownUploads_UploadProjectMarkdown_NoRetryOn5xx verifies that with
		// WithOnlyIdempotentRetries, an upload (a POST) is not retried on a 5xx,
		// which could otherwise store it twice.
		func TestMarkdownUploads_UploadProjectMarkdown_NoRetryOn5xx(t *testing.T) {
		t.Parallel()
		mux, client := setup(t, WithOnlyIdempotentRetries())

		var requests int
		mux.HandleFunc("/api/v4/projects/1/uploads", func(w http.ResponseWriter, r *http.Request) {
		requests++
		http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
		})

		b := strings.NewReader("dummy")
		_, _, err := client.ProjectMarkdownUploads.UploadProjectMarkdown(1, b, "test.txt")
		assert.Error(t, err)
		assert.Equal(t, 1, requests, "the upload must not be retried on a 5xx")
		}

		func TestMarkdownUploads_ListProjectMarkdownUploads(t *testing.T) {
		t.Parallel()
		mux, client := setup(t)