[Bug 63835] Add support for Keep-Alive header

2019-11-16 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #14 from Konstantin Kolinko  ---
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  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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-11-16 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #13 from Konstantin Kolinko  ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-11-15 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #12 from Michael Osipov  ---
The change in Git looks good to me.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-11-15 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #11 from Mark Thomas  ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-11-15 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #10 from Konstantin Kolinko  ---
(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 
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 

[Bug 63835] Add support for Keep-Alive header

2019-11-15 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #9 from Michael Osipov  ---
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:
> 
> > 63835: Add support for Keep-Alive header. (michaelo)
> 
> *response* header

Correct.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-11-14 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #8 from Konstantin Kolinko  ---
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:

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

*response* header

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-11-14 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

Michael Osipov  changed:

   What|Removed |Added

 Resolution|--- |FIXED
 Status|NEW |RESOLVED

--- Comment #7 from Michael Osipov  ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-10-12 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #6 from Michael Osipov  ---
(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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-10-11 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #5 from Mark Thomas  ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-10-11 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #4 from Michael Osipov  ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-10-11 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #3 from Remy Maucherat  ---
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-10-11 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

--- Comment #2 from Michael Osipov  ---
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;

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[Bug 63835] Add support for Keep-Alive header

2019-10-11 Thread bugzilla
https://bz.apache.org/bugzilla/show_bug.cgi?id=63835

Michael Osipov  changed:

   What|Removed |Added

 CC||micha...@apache.org

--- Comment #1 from Michael Osipov  ---
Draft is here:
https://github.com/apache/tomcat/commit/6ff2233cbbd27c9c2c649208a21931e5f3e132a6

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org