Bug 63835 - Add support for Keep-Alive header
Summary: Add support for Keep-Alive header
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Connectors (show other bugs)
Version: 8.5.x-trunk
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-10-11 08:20 UTC by Michael Osipov
Modified: 2019-11-16 12:05 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Osipov 2019-10-11 08:20:01 UTC
There were numerous question on SO and Google in the past years why Tomcat does not send "Keep-Alive: timeout=X, max=Y" just like HTTPd does. Though https://tools.ietf.org/html/draft-thomson-hybi-http-timeout-03 never made it in to an RFC, a lot of clients read that timeout (like our Apache HttpClient) and configure its connection pool or are simply unable to detect terminated connection as described in RFC 7230 and need appropriate hints like this.

I have hit this with a client from a colleague written in C#, actually using a WebServiceClient from .NET. No option to configure details like this.

Adding this header is quite easy since we can take the HTTPd code as a sample: https://github.com/apache/httpd/blob/2.4.x/modules/http/http_protocol.c#L217-L299

I have a prototype implementation ready. If this is accepted I am willing to polish and add appropriate tests for it. I don't see any downsides adding this.
Comment 2 Michael Osipov 2019-10-11 08:53:42 UTC
The implementation contains a bug where the max value must be decreasing. This value can be is available on the endpoint, but there is no getter for. The decrementKeepAliveRequests output is not stored. This would be similar too:

> int left = r->server->keep_alive_max - r->connection->keepalives;
Comment 3 Remy Maucherat 2019-10-11 09:21:40 UTC
This feature idea doesn't look good to me:
- What if there's a proxy ? [usually, there is a proxy]
- This feature looks very late 90s ish and it wasn't added then
Comment 4 Michael Osipov 2019-10-11 10:46:19 UTC
(In reply to Remy Maucherat from comment #3)
> This feature idea doesn't look good to me:
> - What if there's a proxy ? [usually, there is a proxy]
> - This feature looks very late 90s ish and it wasn't added then

This header is a hop-by-hop. If there is a proxy the proxy will read it and adjust. The clieant won't see, but atmost the real values.
I will test this with HTTPd too and see how it behaves.

HTTPd does strip that: https://github.com/apache/httpd/blob/2.4.x/modules/proxy/proxy_util.c#L3780-L3796

Please also consider that this gets activated if and only if "Connection: keep-alive" is sent by the client.
Comment 5 Mark Thomas 2019-10-11 15:25:41 UTC
The proposal never went past draft 03 in 2012. I'm wondering why.

The max parameter is already deprecated in draft 03. I don't think Tomcat should be implementing a deprecated feature of a draft proposal without a very good reason and I can't see one at this point.

I can see the timeout could be useful in avoiding sending a request just as the server was closing the connection, triggering a TCP reset and a need to resend the request. However, for that to work the client needs a reasonable estimate of the latency between the client and the server and that isn't always available.

That it is intended to send this header only when the client sends "Connection: keep-alive" doesn't really change things. Browsers usually send that. Having to parse the request header and likely generate the response header adds overhead to every request. It would be useful to have a sense of the scale of that overhead.
Comment 6 Michael Osipov 2019-10-12 13:15:13 UTC
(In reply to Mark Thomas from comment #5)
> The proposal never went past draft 03 in 2012. I'm wondering why.

Me too.
 
> The max parameter is already deprecated in draft 03. I don't think Tomcat
> should be implementing a deprecated feature of a draft proposal without a
> very good reason and I can't see one at this point.

Agreed, I will happily drop max parameter.

> I can see the timeout could be useful in avoiding sending a request just as
> the server was closing the connection, triggering a TCP reset and a need to
> resend the request. However, for that to work the client needs a reasonable
> estimate of the latency between the client and the server and that isn't
> always available.

Not only latency, you can dynamically adjust your connection pool to that and avoid broken connections. Apache HttpClient does that.

> That it is intended to send this header only when the client sends
> "Connection: keep-alive" doesn't really change things. Browsers usually send
> that. Having to parse the request header and likely generate the response
> header adds overhead to every request. It would be useful to have a sense of
> the scale of that overhead.

The Connection header is parsed anyway with isConnectionClose() already. Getting an integer from the socket wrapper seems easy. I could at most add nanoTime before and after and have a look at these numbers, but I doubt that it will really consume time. WDYT?
Comment 7 Michael Osipov 2019-11-14 22:31:39 UTC
Fixed in:
- master for 9.0.29 onwards
- 8.5.x for 8.5.48 onwards

I have opted not to port back to 7.0.x because a cherry-pick requires significant merge effort. If explicitly requested this can be done later.
Comment 8 Konstantin Kolinko 2019-11-15 07:30:54 UTC
Reviewing the commit implementing this feature in Tomcat 9,
https://github.com/apache/tomcat/commit/1c5bf7a904cffa438eb9b979f3bd32e1579e9666

1. I think that if you are rolling out an experimental feature, there must be a flag controlling it.

2. You must not send this header to a HTTP/1.0 client. (You can skip a lot of code if the client is HTTP/1.0).

3. The code sets a new "Keep-Alive" header unconditionally, regardless of whether one is already present in the response.

(It is unlikely that there is one, but it is one more reason for a flag controlling this feature).

4.
>  if (http11) {
>     headers.addValue(Constants.CONNECTION).setString(Constants.KEEPALIVE);
>  }

Looking at example in [03] page 7, a Connection header in response may already have other connection options, e.g. "Upgrade".

[03] https://tools.ietf.org/html/draft-thomson-hybi-http-timeout-03#page-7

Smoke-testing a Tomcat WebSocket examples with a build from master, I see that Tomcat sends two "Connection" headers:

Connection: upgrade
Connection: keep-alive

Smoke-testing HTTP/2.0, it works (at least in Firefox). Thus it is not a showstopper.

According to RFC 7230 3.2.2. I think it is allowed to generate multiple "Connection" headers.

5.
In Constants.java this commit adds the following constant:

> +     public static final String KEEP_ALIVE = "Keep-Alive";

There already exists Constants.KEEPALIVE constant (without an underscore in the name) in the same class. Adding a second constant with a similar name is confusing.

(As an example:

> isConnectionToken(request.getMimeHeaders(), Constants.KEEPALIVE);

It uses the old lowercase "keep-alive" constant, instead of the new mixed-case one. The isConnectionToken() method expects a lowercase value, thus one missed a chance to create a bug here.
)

6.
>  if (keepAliveTimeout > 0) {
>     String value = "timeout=" + keepAliveTimeout / 1000L;

Can the value be less than 1 second? I think it may be confusing to send back a "timeout=0" value.

7. changelog:

> <bug>63835</bug>: Add support for Keep-Alive header. (michaelo)

*response* header
Comment 9 Michael Osipov 2019-11-15 09:35:06 UTC
Thanks for reviewing, let's go in detail

(In reply to Konstantin Kolinko from comment #8)
> Reviewing the commit implementing this feature in Tomcat 9,
> https://github.com/apache/tomcat/commit/
> 1c5bf7a904cffa438eb9b979f3bd32e1579e9666
> 
> 1. I think that if you are rolling out an experimental feature, there must
> be a flag controlling it.

Why do you consider it to be an experimental feature?

> 2. You must not send this header to a HTTP/1.0 client. (You can skip a lot
> of code if the client is HTTP/1.0).

Can you explain why? I don't see a reason in the 03 draft that this should not be there if the client sends me "Connection: keep-alive"

> 3. The code sets a new "Keep-Alive" header unconditionally, regardless of
> whether one is already present in the response.
> 
> (It is unlikely that there is one, but it is one more reason for a flag
> controlling this feature).

That is true, but why should a webapp component do this? It should not have access to the connector actually. The connector is an implementation detail of the container. HTTPd uses apr_table_setn(), I am doing the same here.

> 4.
> >  if (http11) {
> >     headers.addValue(Constants.CONNECTION).setString(Constants.KEEPALIVE);
> >  }
> 
> Looking at example in [03] page 7, a Connection header in response may
> already have other connection options, e.g. "Upgrade".
> 
> [03] https://tools.ietf.org/html/draft-thomson-hybi-http-timeout-03#page-7
> 
> Smoke-testing a Tomcat WebSocket examples with a build from master, I see
> that Tomcat sends two "Connection" headers:
> 
> Connection: upgrade
> Connection: keep-alive

Granted, that could be "Connection: upgrade, keep-alive", just like apr_table_mergen().

I wonder why our tests don't cover this with WebSockets?! But I have to admit, my WS knowledge is non-existing.

> 5.
> In Constants.java this commit adds the following constant:
> 
> > +     public static final String KEEP_ALIVE = "Keep-Alive";
> 
> There already exists Constants.KEEPALIVE constant (without an underscore in
> the name) in the same class. Adding a second constant with a similar name is
> confusing.

I did this on purpose to have the header name as a verbatim copy of the 03 draft.

> (As an example:
> 
> > isConnectionToken(request.getMimeHeaders(), Constants.KEEPALIVE);
> 
> It uses the old lowercase "keep-alive" constant, instead of the new
> mixed-case one. The isConnectionToken() method expects a lowercase value,
> thus one missed a chance to create a bug here.
> )

No, I don't think so. Mark has completely reworked the parsing and case normalization. I should work case-insensitively. See my ticket for that regard, I have found that edge cases and reported them.

I think you are mixing up up KEEPALIVE and KEEP_ALIVE.

KEEPALIVE: a Connection header value
KEEP_ALIVE: a header name, the constant for the header: Keep-Alive

> 6.
> >  if (keepAliveTimeout > 0) {
> >     String value = "timeout=" + keepAliveTimeout / 1000L;
> 
> Can the value be less than 1 second? I think it may be confusing to send
> back a "timeout=0" value.

That is a very good point I have no real answer for.
What would be your proposal? Change to "keepAliveTimeout > 1000" or ceil to 1?

> 7. changelog:
> 
> > <bug>63835</bug>: Add support for Keep-Alive header. (michaelo)
> 
> *response* header

Correct.
Comment 10 Konstantin Kolinko 2019-11-15 16:55:49 UTC
(In reply to Michael Osipov from comment #9)
> > 
> > 1. I think that if you are rolling out an experimental feature, there must
> > be a flag controlling it.
> 
> Why do you consider it to be an experimental feature?

Two reasons:

1. The specification for this feature is not an officially approved specification.

2. Tomcat 9.0 is declared stable. If you roll-out a new feature like this it is better to have it controllable.

(There may be errors or corner cases in this new implementation. There may be unsuspecting clients. There may already be a configuration for this feature somewhere else - a Filter/Valve inside Tomcat, a Proxy in front of Tomcat, etc., that have to play nicely with it. Finally, it is one of DevOps principles to use feature flags.)

I think that the flag may be removed in some future release, but as the specification for this feature has not been approved yet, I think that such removal will not happen anytime soon.

I do not mind to have this feature "on" by default, thanks to Apache HTTPd 2.4 having tested the feature in the wild.

As this is a protocol feature, I think the flag belongs to a <Connector> configuration.

 
> > 2. You must not send this header to a HTTP/1.0 client. (You can skip a lot
> > of code if the client is HTTP/1.0).
> 
> Can you explain why? I don't see a reason in the 03 draft that this should
> not be there if the client sends me "Connection: keep-alive"

1. I see this as a feature that is targeted to HTTP 1.1 clients.

2. OK, 1.0 clients should not send "Connection: keep-alive" headers, so the version check is redundant, but version check is cheap and allows to skip a lot of code.

(3. Various versions of HTTP/1.1 specification mention that a 1.0 client may send a "Keep-Alive" header for some unclear reason. I was confusing it with "Connection: keep-alive" that is processed here. My fault.)


> > 3. The code sets a new "Keep-Alive" header unconditionally, regardless of
> > whether one is already present in the response.
> > 
> > (It is unlikely that there is one, but it is one more reason for a flag
> > controlling this feature).
> 
> That is true, but why should a webapp component do this? It should not have
> access to the connector actually. The connector is an implementation detail
> of the container. HTTPd uses apr_table_setn(), I am doing the same here.

Instead of implementing this BZ, one may configure a Filter (or Valve) to send such a header.

I am just trying to be compatible.


> > 4.
> > >  if (http11) {
> > >     headers.addValue(Constants.CONNECTION).setString(Constants.KEEPALIVE);
> > >  }
> > 
> > Looking at example in [03] page 7, a Connection header in response may
> > already have other connection options, e.g. "Upgrade".
> > 
> > [03] https://tools.ietf.org/html/draft-thomson-hybi-http-timeout-03#page-7
> > 
> > Smoke-testing a Tomcat WebSocket examples with a build from master, I see
> > that Tomcat sends two "Connection" headers:
> > 
> > Connection: upgrade
> > Connection: keep-alive
> 
> Granted, that could be "Connection: upgrade, keep-alive", just like
> apr_table_mergen().
> 
> I wonder why our tests don't cover this with WebSockets?! But I have to
> admit, my WS knowledge is non-existing.

There are tests for both HTTP/2.0 and WebSockets in the testsuite. Officially, multiple "Connection" headers are functionally equivalent to one header with multiple values. (Whether the tests verify them correctly is a different question.)

The examples web application has WebSocket examples. I use them for smoke tests.

> > 5.
> > In Constants.java this commit adds the following constant:
> > 
> > > +     public static final String KEEP_ALIVE = "Keep-Alive";
> > 
> > There already exists Constants.KEEPALIVE constant (without an underscore in
> > the name) in the same class. Adding a second constant with a similar name is
> > confusing.
> 
> I did this on purpose to have the header name as a verbatim copy of the 03
> draft.

-1. If you like to keep both constants, the new one has to be renamed,
and it would not hurt to have some documentation comment to state the difference.

E.g. KEEP_ALIVE_HEADERNAME, KEEP_ALIVE_HEADER.


> > (As an example:
> > 
> > > isConnectionToken(request.getMimeHeaders(), Constants.KEEPALIVE);
> > 
> > It uses the old lowercase "keep-alive" constant, instead of the new
> > mixed-case one. The isConnectionToken() method expects a lowercase value,
> > thus one missed a chance to create a bug here.
> > )
> 
> No, I don't think so. Mark has completely reworked the parsing and case
> normalization. I should work case-insensitively. See my ticket for that
> regard, I have found that edge cases and reported them.

(The "isConnectionToken()" method is not implemented case-sensitively. It calls HashSet.contains() over a set of lowercase values. - This implementation is OK, as all calls to this private method are known.)

>
> I think you are mixing up up KEEPALIVE and KEEP_ALIVE.
> 
> KEEPALIVE: a Connection header value
> KEEP_ALIVE: a header name, the constant for the header: Keep-Alive

Yes. That is what I am saying: they are confusing. Mixing them up has already happened to me.


> > 6.
> > >  if (keepAliveTimeout > 0) {
> > >     String value = "timeout=" + keepAliveTimeout / 1000L;
> > 
> > Can the value be less than 1 second? I think it may be confusing to send
> > back a "timeout=0" value.
> 
> That is a very good point I have no real answer for.
> What would be your proposal? Change to "keepAliveTimeout > 1000" or ceil to
> 1?

I think that is one more reason to have a flag controlling this feature (per my point #1. above).

Looking at HTTPd code, I think it will happily report back a "timeout=0" value. It is a honest answer and it may be meaningful for some clients. I think that this issue has to be clarified in some future update to the specification.

(Personally, my first thought was to change it to ">= 1000L".)
Comment 11 Mark Thomas 2019-11-15 21:50:36 UTC
Mainly because I want to get 9.0.x and 8.5.x tagged ASAP I am intending to commit fixes to address the concerns raised here shortly.

I agree a rename of the constants would help.

My reading of the code and RFC 7230 is that it is acceptable to send this header to HTTP/1.0 clients when this code does this. The short form of my reasoning is:
- keepAlive is set to false for HTTP/1.0 requests
- only if Connection: keep-alive is present is keepAlive set to true
  (i.e. the HTTP/1.0 client has explicitly advertised keep-alive support)
- this header is only sent if keepAlive is true

I agree that this new feature should be configurable on the Connector, enabled by default. I thought I had stated somewhere I thought this should be configurable but I can't find a reference to that at the moment. Making it configurable also addresses any issues with HTTP/1.0 if the reasoning above is wrong.

I agree the behaviour is "odd" for timeouts < 1000ms. Given the spec doesn't cover this, consistency with httpd (i.e. unchanged from current) as as good as option as any.
Comment 12 Michael Osipov 2019-11-15 23:37:26 UTC
The change in Git looks good to me.
Comment 13 Konstantin Kolinko 2019-11-16 11:23:26 UTC
(In reply to Mark Thomas from comment #11)
> My reading of the code and RFC 7230 is that it is acceptable to send this
> header to HTTP/1.0 clients when this code does this. [...]

Ack. I agree. The recent commit in Git looks good.
Comment 14 Konstantin Kolinko 2019-11-16 12:05:25 UTC
Tes(In reply to Konstantin Kolinko from comment #13)
> (In reply to Mark Thomas from comment #11)
> > My reading of the code and RFC 7230 is that it is acceptable to send this
> > header to HTTP/1.0 clients when this code does this. [...]
> 
> Ack. I agree. The recent commit in Git looks good.

I tested master and 8.5.x, running WebSocket examples. No issues noted. 

Configuration option on <Connector> works correctly. Concatenation of values for Connection header works.

(In reply to Konstantin Kolinko from comment #8)
> 
> Smoke-testing HTTP/2.0, it works (at least in Firefox).
>

HTTP/2.0 works. (It was my error that I expected anything here. The "Upgrade" header is not used when testing with a browser and HTTPS, because negotiation happens with ALPN at connection time.)

Keep-Alive header is not sent with HTTP/2.0, as expected.