https://bz.apache.org/bugzilla/show_bug.cgi?id=63835
--- Comment #10 from Konstantin Kolinko <knst.koli...@gmail.com> ---
(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".)
--
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
