Skip to content

Make timeout per-request - #1181

Merged
ktock merged 1 commit into
containerd:mainfrom
Kern--:main
Mar 28, 2023
Merged

ktock merged 1 commit into
containerd:mainfrom
Kern--:main

Conversation

@Kern--

@Kern-- Kern-- commented Mar 27, 2023 •

Copy link
Copy Markdown

Part of #1180

When setting up an rhttp client, the structure looks like this:

http.Client {
    Transport: rhttp.Transport {
        Client: rhttp.Client {
            HTTPClient: http.Client{}
        }
    }
}

Before this change, the timeout was set on the outer client. When the timeout was reached, the request context was cancelled and no more retries would be attempted by the retryable client. Effectively the timeout was being set for the entire retryable request.

This change moves the timeout to the inner client, making the timeout per-request and the timeout of the entire retryable request is governed by the retry policy.

@ktock

ktock commented Mar 28, 2023

Copy link
Copy Markdown
Member

@Kern-- Thank you for the patch, looks good to me. Could you rebase thie patch on the latest main branch for fixing the CI failure?

When setting up an rhttp client, the structure looks like this:
```
http.Client {
    Transport: rhttp.Transport {
        Client: rhttp.Client {
            HTTPClient: http.Client{}
        }
    }
}
```

Before this change, the timeout was set on the outer client. When the
timeout was reached, the request context was cancelled and no more
retries would be attempted by the retryable client. Effectively the
timeout was being set for the entire retryable request.

This change moves the timeout to the inner client, making the timeout
per-request and the timeout of the entire retryable request is governed
by the retry policy.

Signed-off-by: Kern Walster <walster@amazon.com>
@ktock

ktock commented Mar 28, 2023

Copy link
Copy Markdown
Member

Thanks!

@ktock
ktock merged commit d3db79c into containerd:main Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants