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