Make cancelation configurable

[samsp-msft]

[Re-filing as I screwed up and converted to a discussion when I did it in the wrong tab. Originally #293]

Offline feedback from the StackOverflow team.

Two cancellation tokens are used when proxying requests, a short one and a long one.

reverse-proxy/src/ReverseProxy/Middleware/ProxyInvokerMiddleware.cs

Lines 76 to 87 in a3b5e1b

	// TODO: Configurable timeout, measure from request start, make it unit-testable
	shortCts.CancelAfter(TimeSpan.FromSeconds(30));
	// TODO: Retry against other destinations
	try
	{
	// TODO: Apply caps
	cluster.ConcurrencyCounter.Increment();
	destination.ConcurrencyCounter.Increment();
	// TODO: Duplex channels should not have a timeout (?), but must react to Proxy force-shutdown signals.
	var longCancellation = context.RequestAborted;

The short token defaults to a 30 second timeout and is not currently configurable. This tracks the amount of time the destination has to respond with headers (not body). It's also linked to the long token.

• This timeout should be configurable globally
• Should there be a way to override it per route or cluster?

The long token is actually the HttpContext.RequestAborted token. It's used to track the response body. This tells the destination to abort if the client disconnects.

• Does this need to be configurable in any way?
• Should there also be a timeout applied to this part of the request? Configurable at what level?
• Timeouts on the body don't work well for varying sized responses, or for streaming scenarios.
• Middleware could add its own timeouts per request by augmenting RequestAborted.

[samsp-msft]

@scalablecory, @alnikola - why does the 30s timeout seem familiar for something baked into the networking stack?

[davidni]

Should there be a way to override it per route or cluster?

Yes! :). We recently implemented configurability at the Cluster level by adding a top level property ProxyTimeout on ClusterConfig. Being able to set it per route would also be useful. Order of precedence IMHO should be RouteConfig --> ClusterConfig --> Global value.

[davidni]

Related: When retries are implemented, it will be important to further distinguish timeout per attempt, and global timeout for all attempts.

Additional reading: Timeouts in Envoy, per_try_timeout

[scalablecory]

@scalablecory, @alnikola - why does the 30s timeout seem familiar for something baked into the networking stack?

Doesn't ring any bells to me. Could you be thinking of the ~60 second Socket.Connect() timeout in Windows?

[amweiss]

Per-route would be ideal for our use cases, but per-cluster would be sufficient.

[NickCraver]

Could we start with the global timeout and potentially do the others later on? The static 30 is causing constant issues for us, especially on first request when the app behind the proxy is spinning up.

[Tratcher]

@NickCraver How about I start by bumping it to 100s to match HttpClient's traditional default?

[NickCraver]

@Tratcher that would definitely help - practically speaking we wouldn't need to adjust it for the 95% case after that. Until we can configure it, that'd be a very welcomed change! /cc @deanward81

[Tratcher]

@NickCraver ok, I'll tweak it in #406 for preview5 since I'm about to mess with timeouts anyways.

[ManickaP]

Fixed in #512