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

responseHeaderTimeout should be < idleTimeout #2550

Open
RafalSumislawski opened this issue May 3, 2019 · 3 comments

Comments

@RafalSumislawski
Copy link
Contributor

commented May 3, 2019

The BlazeServerSpec says "A server should return a 503 if the server doesn't respond" and it works like that in the test, but it does only because response header time-out is set to 1 second. The default for responseHeaderTimeout is 1 minute while the default for idleTimeout is 30 seconds (see: BlazeServerBuilder ). That means that when running with default configuration, the idleTimeout kicks in first, and instead of the nice behaviour of returning HTTP 503 response, we get an abrupt TCP connection termination, leaving the client with no clue what went wrong.

IMO the defaults for the timeouts should be changed so that the responseHeaderTimeout is shorter than the idleTimeout. In addition to that, the blaze server could log a warning it is started with responseHeaderTimeout >= idleTimeout.

@rossabaker

This comment has been minimized.

Copy link
Member

commented May 29, 2019

I agree with all of this, but what do we propose as the new values?

@RafalSumislawski

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

I did some research on default timeouts used in other http client/server libraries:

  • akka-http
    • client-side: connecting-timeout = 10s
    • client-side: request-timeout not implemented
    • server-side: request-timeout = 20s
    • client-side: idle-timeout = 60 s
    • server-side: idle-timeout = 60 s
  • [lihaoyi/requests-scala] (https://github.com/lihaoyi/requests-scala/blob/0c654ef3948e8e29c67d968ab4d08c325c1663ea/requests/src/requests/package.scala)
    • client-side: connectTimeout = 10s
    • client-side: readTimeout = 10s
  • sttp
    • client-side: Connection timeout = 30s
    • client-side: Read timeout = 60s
  • async-http-client
    • client-side: connectTimeout = 5s
    • client-side: pooledConnectionIdleTimeout = 60s
    • client-side: readTimeout = 60s
    • client-side: requestTimeout = 60s
  • OkHttp client
    • client-side: connectTimeout = 10s
    • client-side: readTimeout = 10s
  • Tomcat 9
    • server-side: asyncTimeout = 30s // simillar to responseHeaderTimeout
  • Jetty 9 Server / client
    • server-side: idleTimeout = 30s
    • client-side: connectTimeout = 15s
    • client-side: idleTimeout = 0 (no timeout)
    • client-side: requestTimeout = 0 (no timeout)

As far as I understand readTimeouts work similarly to responseHeaderTimeout.

Current http4s blaze defaults:

  • client-side: responseHeaderTimeout = 10.seconds,
  • server-side: responseHeaderTimeout = 1.minute
  • client-side: requestTimeout = 1.minute,
  • client-side: idleTimeout = 1.minute,
  • server-side: idleTimeout = 30.seconds

I would like to propose following values for http4s blaze defaults

  • client-side: connectingTimeout = 10.seconds, // coming soon
  • client-side: responseHeaderTimeout = Duration.Inf, // simplifies configuration
  • server-side: responseHeaderTimeout = 30.seconds // same as tomcat and jetty
  • client-side: requestTimeout = 45.seconds, // shorter than idle timeout, long enough to receive timeout responses from tomcat, jetty and akka-http
  • client-side: idleTimeout = 60.seconds,
  • server-side: idleTimeout = 60.seconds
    In addition to that I would like to add runtime warnings in case responseHeaderTimeout or requestTimeout is larger than idleTimeout.

A note on responseHeaderTimeout:
Http4s client has two timeouts which are designed to cancel a request when it's taking too long. The responseHeaderTimeout and the requestTimeout. responseHeaderTimeout handles only a subset of scenarios handled requestTimeout. It is reasonable to use the client with just requestTimeout, but it's unreasonable to use the client with just responseHeaderTimeout. For example if the servers starts sending the response early, but it's sending it very slowly, the requestTimeout will kick-in, the responseHeaderTimeout wouldn't. That doesn't mean that responseHeaderTimeout is useless, rather that it's an advanced feature, not necessary in basic setup. Setting responseHeaderTimeout to Inf by default, makes timeout configuration more straightforward.

A note on idleTimeout:
Idle timeouts result in abrupt TCP connection termination. IMO they should be used for:

  • shrinking of connection pools.
  • protecting against resource leaks

but not for:

  • business-level control of request execution time
@RafalSumislawski

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

@rossabaker
I'll wait from some comments/discussion in this issue. Once everything is clear, I can provide a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.