Re: [tomcat] 01/01: First draft

2019-10-11 Thread Michael Osipov

Am 2019-10-11 um 14:35 schrieb Rémy Maucherat:

On Fri, Oct 11, 2019 at 1:49 PM Michael Osipov  wrote:


Am 2019-10-11 um 11:51 schrieb Rémy Maucherat:

On Fri, Oct 11, 2019 at 10:30 AM  wrote:


This is an automated email from the ASF dual-hosted git repository.

michaelo pushed a commit to branch BZ-63835/8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git

commit 6ff2233cbbd27c9c2c649208a21931e5f3e132a6
Author: Michael Osipov 
AuthorDate: Fri Oct 11 10:30:08 2019 +0200

  First draft



+if (keepAliveTimeout > 0) {
+String value = "timeout=" +
TimeUnit.MILLISECONDS.toSeconds(keepAliveTimeout);
+
+if (maxKeepAliveRequests > 0) {
+value += ", max=" + maxKeepAliveRequests;
+}
StringBuilder ?


StringBuilder does not make any sense because:

* The compiler will replace the static code automatically with a
StringBuilder
* StringBuilder gains benefit when you concat strings in a for/while/do
loop. The above code is purely static.



I don't understand how this can work, or how it is static, but if you're
100% certain it's fine.


Please look here: 
https://dzone.com/articles/string-concatenation-performacne-improvement-in-ja


Exactly the same case.


Can you add a new flag controlling the feature ? The information may not

be

very useful in many cases such as when proxying, so it would be better to
skip generating it by default.


This is -- as documented -- a first draft.

As mentioned on the ticket. This is hop-by-hop and writetn only if the
client requests this piece of information. We can surely discuss a flag
for this on the connector.



Ok indeed. I never understood why some clients kept sending Connection:
keep-alive since it was also not needed.


To be honest, I didn't know either until I started digging for the 
client problem a colleague had.


Try against HTTPd and you'll see even with HTTP 1.1 client.


Michael


-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



Re: [tomcat] 01/01: First draft

2019-10-11 Thread Rémy Maucherat
On Fri, Oct 11, 2019 at 1:49 PM Michael Osipov  wrote:

> Am 2019-10-11 um 11:51 schrieb Rémy Maucherat:
> > On Fri, Oct 11, 2019 at 10:30 AM  wrote:
> >
> >> This is an automated email from the ASF dual-hosted git repository.
> >>
> >> michaelo pushed a commit to branch BZ-63835/8.5.x
> >> in repository https://gitbox.apache.org/repos/asf/tomcat.git
> >>
> >> commit 6ff2233cbbd27c9c2c649208a21931e5f3e132a6
> >> Author: Michael Osipov 
> >> AuthorDate: Fri Oct 11 10:30:08 2019 +0200
> >>
> >>  First draft
> >>
> >
> > +if (keepAliveTimeout > 0) {
> > +String value = "timeout=" +
> > TimeUnit.MILLISECONDS.toSeconds(keepAliveTimeout);
> > +
> > +if (maxKeepAliveRequests > 0) {
> > +value += ", max=" + maxKeepAliveRequests;
> > +}
> > StringBuilder ?
>
> StringBuilder does not make any sense because:
>
> * The compiler will replace the static code automatically with a
> StringBuilder
> * StringBuilder gains benefit when you concat strings in a for/while/do
> loop. The above code is purely static.
>

I don't understand how this can work, or how it is static, but if you're
100% certain it's fine.


>
> > Can you add a new flag controlling the feature ? The information may not
> be
> > very useful in many cases such as when proxying, so it would be better to
> > skip generating it by default.
>
> This is -- as documented -- a first draft.
>
> As mentioned on the ticket. This is hop-by-hop and writetn only if the
> client requests this piece of information. We can surely discuss a flag
> for this on the connector.
>

Ok indeed. I never understood why some clients kept sending Connection:
keep-alive since it was also not needed.

Rémy


Re: [tomcat] 01/01: First draft

2019-10-11 Thread Michael Osipov

Am 2019-10-11 um 11:51 schrieb Rémy Maucherat:

On Fri, Oct 11, 2019 at 10:30 AM  wrote:


This is an automated email from the ASF dual-hosted git repository.

michaelo pushed a commit to branch BZ-63835/8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git

commit 6ff2233cbbd27c9c2c649208a21931e5f3e132a6
Author: Michael Osipov 
AuthorDate: Fri Oct 11 10:30:08 2019 +0200

 First draft



+if (keepAliveTimeout > 0) {
+String value = "timeout=" +
TimeUnit.MILLISECONDS.toSeconds(keepAliveTimeout);
+
+if (maxKeepAliveRequests > 0) {
+value += ", max=" + maxKeepAliveRequests;
+}
StringBuilder ?


StringBuilder does not make any sense because:

* The compiler will replace the static code automatically with a 
StringBuilder
* StringBuilder gains benefit when you concat strings in a for/while/do 
loop. The above code is purely static.



Can you add a new flag controlling the feature ? The information may not be
very useful in many cases such as when proxying, so it would be better to
skip generating it by default.


This is -- as documented -- a first draft.

As mentioned on the ticket. This is hop-by-hop and writetn only if the 
client requests this piece of information. We can surely discuss a flag 
for this on the connector.



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



Re: [tomcat] 01/01: First draft

2019-10-11 Thread Rémy Maucherat
On Fri, Oct 11, 2019 at 10:30 AM  wrote:

> This is an automated email from the ASF dual-hosted git repository.
>
> michaelo pushed a commit to branch BZ-63835/8.5.x
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
> commit 6ff2233cbbd27c9c2c649208a21931e5f3e132a6
> Author: Michael Osipov 
> AuthorDate: Fri Oct 11 10:30:08 2019 +0200
>
> First draft
>

+if (keepAliveTimeout > 0) {
+String value = "timeout=" +
TimeUnit.MILLISECONDS.toSeconds(keepAliveTimeout);
+
+if (maxKeepAliveRequests > 0) {
+value += ", max=" + maxKeepAliveRequests;
+}
StringBuilder ?

Can you add a new flag controlling the feature ? The information may not be
very useful in many cases such as when proxying, so it would be better to
skip generating it by default.

Rémy


> ---
>  java/org/apache/coyote/http11/Http11Processor.java | 36
> ++
>  1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/java/org/apache/coyote/http11/Http11Processor.java
> b/java/org/apache/coyote/http11/Http11Processor.java
> index 6072602..3182bb7 100644
> --- a/java/org/apache/coyote/http11/Http11Processor.java
> +++ b/java/org/apache/coyote/http11/Http11Processor.java
> @@ -23,6 +23,7 @@ import java.util.Enumeration;
>  import java.util.Locale;
>  import java.util.Map;
>  import java.util.Set;
> +import java.util.concurrent.TimeUnit;
>  import java.util.regex.Pattern;
>
>  import javax.servlet.http.HttpServletResponse;
> @@ -1307,7 +1308,7 @@ public class Http11Processor extends
> AbstractProcessor {
>  } else {
>  // If the response code supports an entity body and we're on
>  // HTTP 1.1 then we chunk unless we have a Connection: close
> header
> -connectionClosePresent = isConnectionClose(headers);
> +connectionClosePresent = isConnectionValue(headers,
> Constants.CLOSE);
>  if (entityBody && http11 && !connectionClosePresent) {
>  outputBuffer.addActiveFilter
>  (outputFilters[Constants.CHUNKED_FILTER]);
> @@ -1369,10 +1370,33 @@ public class Http11Processor extends
> AbstractProcessor {
>  headers.addValue(Constants.CONNECTION).setString(
>  Constants.CLOSE);
>  }
> -} else if (!http11 && !getErrorState().isError()) {
> -
> headers.addValue(Constants.CONNECTION).setString(Constants.KEEPALIVE);
> -}
> +} else if (!getErrorState().isError()) {
> +if (!http11) {
> +
> headers.addValue(Constants.CONNECTION).setString(Constants.KEEPALIVE);
> +}
> +
> +boolean connectionKeepAlivePresent =
> +isConnectionValue(request.getMimeHeaders(),
> Constants.KEEPALIVE);
> +
> +if (connectionKeepAlivePresent) {
> +int keepAliveTimeout = endpoint.getKeepAliveTimeout();
> +int maxKeepAliveRequests = getMaxKeepAliveRequests();
> +
> +if (keepAliveTimeout > 0) {
> +String value = "timeout=" +
> TimeUnit.MILLISECONDS.toSeconds(keepAliveTimeout);
> +
> +if (maxKeepAliveRequests > 0) {
> +value += ", max=" + maxKeepAliveRequests;
> +}
>
> +headers.setValue("Keep-Alive").setString(value);
> +}
> +
> +if (http11) {
> +
> headers.addValue(Constants.CONNECTION).setString(Constants.KEEPALIVE);
> +}
> +}
> +}
>  // Add server header
>  if (server == null) {
>  if (serverRemoveAppProvidedValues) {
> @@ -1403,12 +1427,12 @@ public class Http11Processor extends
> AbstractProcessor {
>  outputBuffer.commit();
>  }
>
> -private static boolean isConnectionClose(MimeHeaders headers) {
> +private static boolean isConnectionValue(MimeHeaders headers, String
> value) {
>  MessageBytes connection = headers.getValue(Constants.CONNECTION);
>  if (connection == null) {
>  return false;
>  }
> -return connection.equals(Constants.CLOSE);
> +return connection.equals(value);
>  }
>
>  private void prepareSendfile(OutputFilter[] outputFilters) {
>
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
>