https://bz.apache.org/bugzilla/show_bug.cgi?id=63835
--- Comment #9 from Michael Osipov <micha...@apache.org> --- 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. -- 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
