Skip to content

Support a separate timeout for listing pending requests #149

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
May 13, 2025
Merged

Support a separate timeout for listing pending requests #149

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
043a010
Support a custom list-pending-requests timeout
kenfreeman May 5, 2025
5479066
Revert "Support a custom list-pending-requests timeout"
kenfreeman May 6, 2025
a129f4e
Support a custom list-pending-requests timeout
kenfreeman May 6, 2025
2dc4b43
Support a custom list-pending-requests timeout
kenfreeman May 6, 2025
8bcfe0d
Support a custom list-pending-requests timeout
kenfreeman May 6, 2025
ca84ddf
Support a custom list-pending-requests timeout
kenfreeman May 6, 2025
e402686
Support a custom list-pending-requests timeout
kenfreeman May 6, 2025
da428e5
Support a custom list-pending-requests timeout
kenfreeman May 6, 2025
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
11 changes: 10 additions & 1 deletion agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ const (
var (
proxy = flag.String("proxy", "", "URL (including scheme) of the inverting proxy")
proxyTimeout = flag.Duration("proxy-timeout", 60*time.Second, "Client timeout when sending requests to the inverting proxy")
listRequestsTimeout = flag.Duration("list-requests-timeout", 0*time.Second, "Client timeout for listing requests from the inverting proxy. Defaults to proxy-timeout value")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Implementations of the proxy have to coordinate with the existing proxy-timeout flag specifically for the purposes of the calls to list pending requests (the proxy has to timeout those calls before the agent times them out or else there is a risk of requests getting dropped).

As such, I'd rather that we keep using the proxy-timeout flag value for the calls to list pending requests, and make the new flag be used for all the other calls.

E.G. the new flag could be something like proxy-client-timeout and be used where the proxyTimeout variable was previously used.

That being said, we would want to update the doc string for the existing proxy-timeout flag to indicate its update scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I went in the direction I did is because users will set their timeouts based on the expected load and latency of their backends. So it made sense to me to keep the existing flag for that purpose.

That said, I'm happy to make the change you suggest. I just want to be sure that any users who don't set the new value get the current behavior. So we could create a proxy-client-timeout but use proxy-timeout if proxy-client-timeout isn't set.

I am a bit confused by the terminology. What is a "proxy client"?

I see the proxy-agent doing 3 things: (1) getting a list of pending requests from the IVP; (2) Pulling an individual request from the IVP; (3) Forwarding a request to a backend and waiting for a result. I think #1 and #2 are both proxy clients, since they call the IVP URL.

(I understand #3 goes through a host proxy, but I think of that as an implementation detail. When I hear "proxy" in this context, I think of just the IVP.)

Would it make sense to support 3 separate timeouts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a bit confused by the terminology. What is a "proxy client"?

This would be used as the timeout set on the HTTP client used to communicate with the proxy.

I just want to be sure that any users who don't set the new value get the current behavior.

Agreed. I think the best way to do that is to require the value of the new flag to be larger than the value of the existing flag, explicitly raising it if it is not.

Would it make sense to support 3 separate timeouts?

I don't think so. The agent makes three types of calls to the Inverting Proxy:

  1. List the IDs of pending requests
  2. Fetch the contents of a single request
  3. Post the response for a single request

You could think of this as two broad actions:

  1. Poll for new requests
  2. Process a single request (read it and then post back the response)

So, having two separate timeouts makes sense, but having three would be overkill because the two other actions are both just part of processing a single request.

So, an alternative might be to name the new flag something like request-timeout or request-forwarding-timeout.

Thinking about it more, I think I like that better because it makes the existing proxy-timeout name make more sense in comparison, as in one case we are waiting for an individual request/response, but in the other we are only waiting on the Inverting Proxy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. In short, the change would:

  1. Define a new flag called request-forwarding-timeout with a default value of 0
  2. Set its value to the proxy timeout if no value was provided, or if a value less than the proxy timeout was provided
  3. Use proxy timeout for getting the list of pending requests. Implement this with a context with a timeout.
  4. Use the new request-forwarding-timeout for reading requests and posting the response. This can be implemented by setting a timeout on the HTTP connection.
  5. Change the help message on proxy-timeout from "Client timeout when sending requests to the inverting proxy" to "Timeout for polling the inverting proxy for new requests" or similar
  6. Add a help message on request-forwarding-timeout to something like "Timeout for forwarding individual requests to the backend and returning a response"

WDYT?

host = flag.String("host", "localhost:8080", "Hostname (including port) of the backend server")
forceHTTP2 = flag.Bool("force-http2", false, "Force connections to the backend host to be performed using HTTP/2")
backendID = flag.String("backend", "", "Unique ID for this backend.")
Expand Down Expand Up @@ -205,14 +206,22 @@ func processOneRequest(client *http.Client, hostProxy http.Handler, backendID st
func pollForNewRequests(pollingCtx context.Context, client *http.Client, hostProxy http.Handler, backendID string) {
previouslySeenRequests := lru.New(requestCacheLimit)

// If listRequestsTimeout is set and is less than proxyTimeout, use it. Otherwise, use proxyTimeout.
listRequestsTimeoutToUse := *proxyTimeout
if *listRequestsTimeout > 0 && *listRequestsTimeout < *proxyTimeout {
listRequestsTimeoutToUse = *listRequestsTimeout
}

var retryCount uint
for {
select {
case <-pollingCtx.Done():
log.Printf("Request polling context completed with ctx err: %v\n", pollingCtx.Err())
return
default:
if requests, err := utils.ListPendingRequests(client, *proxy, backendID, metricHandler); err != nil {
listRequestsCtx, cancel := context.WithTimeout(pollingCtx, listRequestsTimeoutToUse)
defer cancel()
if requests, err := utils.ListPendingRequests(listRequestsCtx, client, *proxy, backendID, metricHandler); err != nil {
log.Printf("Failed to read pending requests: %q\n", err.Error())
time.Sleep(utils.ExponentialBackoffDuration(retryCount))
retryCount++
Expand Down
4 changes: 2 additions & 2 deletions agent/utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,9 +230,9 @@ func RoundTripperWithVMIdentity(ctx context.Context, wrapped http.RoundTripper,
}

// ListPendingRequests issues a single request to the proxy to ask for the IDs of pending requests.
func ListPendingRequests(client *http.Client, proxyHost, backendID string, metricHandler *metrics.MetricHandler) ([]string, error) {
func ListPendingRequests(ctx context.Context, client *http.Client, proxyHost, backendID string, metricHandler *metrics.MetricHandler) ([]string, error) {
proxyURL := proxyHost + PendingPath
proxyReq, err := http.NewRequest(http.MethodGet, proxyURL, nil)
proxyReq, err := http.NewRequestWithContext(ctx, http.MethodGet, proxyURL, nil)
if err != nil {
return nil, err
}
Expand Down