Re: [tomcat] branch main updated: Implement the new connection ID and request ID API for Servlet 6.0
On 24/09/2021 20:27, Christopher Schultz wrote: Mark, On 9/24/21 14:39, Mark Thomas wrote: On 24/09/2021 19:21, Christopher Schultz wrote: Mark, Sorry for the top-post but this is quite a long patch and it will be easier to find my comments up here. No problem. Makes sense. When generating the String value for the request identifier, you could use: private String requestId = Long.toHexString(requestIdGenerator.getAndIncrement()); This would produce smaller strings, use slightly less CPU, and then nobody cares whether the value is - or +. Excellent. I like it. I'll make the change. We might need those extra 3,000,000 years before failover ;) Should the new request-id be generated during recycle() or during another time? Is there a method which is always called to re-commission a Request object? If so, I think it would make more sense to leave the requestId alone in recycle() and allocate the new request-id in that other method. Not sure. I'll take a look. In cases where there are use-after-recycle errors in an application, having the old request-id might be helpful in debugging. I think that could go either way. The ID changing as soon as recycle is called might make use after recycle easier to spot. The more I think about this, the more I think changing ID on recycle is the way to go but I am prepared to be convinced otherwise. How about requestId = -1 on recycle (and create) but set it to a definite value when it's "in service"? Whilst I like the idea of being able to tell from the ID that the request is recycled, at the back of my mind are some historical bugs where we have two Processors (and hence 2 Requests) assigned to the same connection at the same time. I think always having a unique ID for the request is important for debugging that use case. Your suggestion got me thinking though. It might be possible to ensure that once the request has been recycled (and the new ID issued) that subsequent calls to recycle before the start of the next request don't trigger an increment in the ID. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch main updated: Implement the new connection ID and request ID API for Servlet 6.0
Mark, On 9/24/21 14:39, Mark Thomas wrote: On 24/09/2021 19:21, Christopher Schultz wrote: Mark, Sorry for the top-post but this is quite a long patch and it will be easier to find my comments up here. No problem. Makes sense. When generating the String value for the request identifier, you could use: private String requestId = Long.toHexString(requestIdGenerator.getAndIncrement()); This would produce smaller strings, use slightly less CPU, and then nobody cares whether the value is - or +. Excellent. I like it. I'll make the change. We might need those extra 3,000,000 years before failover ;) Should the new request-id be generated during recycle() or during another time? Is there a method which is always called to re-commission a Request object? If so, I think it would make more sense to leave the requestId alone in recycle() and allocate the new request-id in that other method. Not sure. I'll take a look. In cases where there are use-after-recycle errors in an application, having the old request-id might be helpful in debugging. I think that could go either way. The ID changing as soon as recycle is called might make use after recycle easier to spot. The more I think about this, the more I think changing ID on recycle is the way to go but I am prepared to be convinced otherwise. How about requestId = -1 on recycle (and create) but set it to a definite value when it's "in service"? -chris - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch main updated: Implement the new connection ID and request ID API for Servlet 6.0
On 24/09/2021 19:21, Christopher Schultz wrote: Mark, Sorry for the top-post but this is quite a long patch and it will be easier to find my comments up here. No problem. Makes sense. When generating the String value for the request identifier, you could use: private String requestId = Long.toHexString(requestIdGenerator.getAndIncrement()); This would produce smaller strings, use slightly less CPU, and then nobody cares whether the value is - or +. Excellent. I like it. I'll make the change. We might need those extra 3,000,000 years before failover ;) Should the new request-id be generated during recycle() or during another time? Is there a method which is always called to re-commission a Request object? If so, I think it would make more sense to leave the requestId alone in recycle() and allocate the new request-id in that other method. Not sure. I'll take a look. In cases where there are use-after-recycle errors in an application, having the old request-id might be helpful in debugging. I think that could go either way. The ID changing as soon as recycle is called might make use after recycle easier to spot. The more I think about this, the more I think changing ID on recycle is the way to go but I am prepared to be convinced otherwise. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch main updated: Implement the new connection ID and request ID API for Servlet 6.0
On 24/09/2021 19:03, Christopher Schultz wrote: Mark, I haven't looked at the patch yet but I was thinking about this since you mentioned it at ApacheCon. Many load-balancers and other similar network systems are capable of generating their own request-identifiers and sending them as request headers to origin servers. I think it would make sense to allow the user to either use a container-generated request identifier (as you have implemented it) or to allow a (trusted) upstream component to provide one to the container. WDYT? I don't think that covers all use cases as it requires the successful parsing of the incoming request. Users are free to log multiple IDs from different sources if that makes sense for their environment. These IDs are primarily to fill the gap that there wasn't IDs from the container perspective. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch main updated: Implement the new connection ID and request ID API for Servlet 6.0
Mark,
Sorry for the top-post but this is quite a long patch and it will be
easier to find my comments up here.
When generating the String value for the request identifier, you could use:
private String requestId =
Long.toHexString(requestIdGenerator.getAndIncrement());
This would produce smaller strings, use slightly less CPU, and then
nobody cares whether the value is - or +.
This field cannot be FINAL because request objects can be re-used. Okay.
Should the new request-id be generated during recycle() or during
another time? Is there a method which is always called to re-commission
a Request object? If so, I think it would make more sense to leave the
requestId alone in recycle() and allocate the new request-id in that
other method.
In cases where there are use-after-recycle errors in an application,
having the old request-id might be helpful in debugging.
-chris
On 9/24/21 13:30, ma...@apache.org wrote:
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new b7c05a8 Implement the new connection ID and request ID API for
Servlet 6.0
b7c05a8 is described below
commit b7c05a8f60003c42e6f367bf307188ce391dbad2
Author: Mark Thomas
AuthorDate: Fri Sep 24 18:30:24 2021 +0100
Implement the new connection ID and request ID API for Servlet 6.0
---
java/jakarta/el/ImportHandler.java | 1 +
java/jakarta/servlet/ServletConnection.java| 95 +++
java/jakarta/servlet/ServletRequest.java | 48
java/jakarta/servlet/ServletRequestWrapper.java| 36 ++
java/org/apache/catalina/Globals.java | 16 ---
java/org/apache/catalina/connector/Request.java| 48
.../apache/catalina/connector/RequestFacade.java | 19 +++
java/org/apache/coyote/AbstractProcessor.java | 37 +++---
java/org/apache/coyote/ActionCode.java | 13 ++-
java/org/apache/coyote/Request.java| 39 ++-
java/org/apache/coyote/ajp/AjpProcessor.java | 7 ++
java/org/apache/coyote/http11/Http11Processor.java | 7 ++
.../coyote/http2/Http2AsyncUpgradeHandler.java | 4 +-
java/org/apache/coyote/http2/Http2Protocol.java| 4 +-
.../apache/coyote/http2/Http2UpgradeHandler.java | 20 +++-
java/org/apache/coyote/http2/StreamProcessor.java | 18 +--
.../tomcat/util/net/ServletConnectionImpl.java | 55 +
.../apache/tomcat/util/net/SocketWrapperBase.java | 30 +
.../catalina/filters/TesterHttpServletRequest.java | 16 +++
.../apache/coyote/http2/TestAbstractStream.java| 130 +++--
webapps/docs/changelog.xml | 4 +
21 files changed, 553 insertions(+), 94 deletions(-)
diff --git a/java/jakarta/el/ImportHandler.java
b/java/jakarta/el/ImportHandler.java
index 138a6da..b824d5d 100644
--- a/java/jakarta/el/ImportHandler.java
+++ b/java/jakarta/el/ImportHandler.java
@@ -54,6 +54,7 @@ public class ImportHandler {
servletClassNames.add("RequestDispatcher");
servletClassNames.add("Servlet");
servletClassNames.add("ServletConfig");
+servletClassNames.add("ServletConnection");
servletClassNames.add("ServletContainerInitializer");
servletClassNames.add("ServletContext");
servletClassNames.add("ServletContextAttributeListener");
diff --git a/java/jakarta/servlet/ServletConnection.java
b/java/jakarta/servlet/ServletConnection.java
new file mode 100644
index 000..97ded16
--- /dev/null
+++ b/java/jakarta/servlet/ServletConnection.java
@@ -0,0 +1,95 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one or more
+* contributor license agreements. See the NOTICE file distributed with
+* this work for additional information regarding copyright ownership.
+* The ASF licenses this file to You under the Apache License, Version 2.0
+* (the "License"); you may not use this file except in compliance with
+* the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package jakarta.servlet;
+
+/**
+ * Provides information about the connection made to the Servlet container.
This
+ * interface is intended primarily for debugging purposes and as such provides
+ * the raw information as seen by the container. Unless explicitly stated
+ * otherwise in the Javadoc for a method, no adjustment is made for the
presence
+ * of reverse proxies or similar configurations.
+ *
+ * @since Se
Re: [tomcat] branch main updated: Implement the new connection ID and request ID API for Servlet 6.0
Mark,
I haven't looked at the patch yet but I was thinking about this since
you mentioned it at ApacheCon.
Many load-balancers and other similar network systems are capable of
generating their own request-identifiers and sending them as request
headers to origin servers. I think it would make sense to allow the user
to either use a container-generated request identifier (as you have
implemented it) or to allow a (trusted) upstream component to provide
one to the container.
WDYT?
-chris
On 9/24/21 13:30, ma...@apache.org wrote:
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new b7c05a8 Implement the new connection ID and request ID API for
Servlet 6.0
b7c05a8 is described below
commit b7c05a8f60003c42e6f367bf307188ce391dbad2
Author: Mark Thomas
AuthorDate: Fri Sep 24 18:30:24 2021 +0100
Implement the new connection ID and request ID API for Servlet 6.0
---
java/jakarta/el/ImportHandler.java | 1 +
java/jakarta/servlet/ServletConnection.java| 95 +++
java/jakarta/servlet/ServletRequest.java | 48
java/jakarta/servlet/ServletRequestWrapper.java| 36 ++
java/org/apache/catalina/Globals.java | 16 ---
java/org/apache/catalina/connector/Request.java| 48
.../apache/catalina/connector/RequestFacade.java | 19 +++
java/org/apache/coyote/AbstractProcessor.java | 37 +++---
java/org/apache/coyote/ActionCode.java | 13 ++-
java/org/apache/coyote/Request.java| 39 ++-
java/org/apache/coyote/ajp/AjpProcessor.java | 7 ++
java/org/apache/coyote/http11/Http11Processor.java | 7 ++
.../coyote/http2/Http2AsyncUpgradeHandler.java | 4 +-
java/org/apache/coyote/http2/Http2Protocol.java| 4 +-
.../apache/coyote/http2/Http2UpgradeHandler.java | 20 +++-
java/org/apache/coyote/http2/StreamProcessor.java | 18 +--
.../tomcat/util/net/ServletConnectionImpl.java | 55 +
.../apache/tomcat/util/net/SocketWrapperBase.java | 30 +
.../catalina/filters/TesterHttpServletRequest.java | 16 +++
.../apache/coyote/http2/TestAbstractStream.java| 130 +++--
webapps/docs/changelog.xml | 4 +
21 files changed, 553 insertions(+), 94 deletions(-)
diff --git a/java/jakarta/el/ImportHandler.java
b/java/jakarta/el/ImportHandler.java
index 138a6da..b824d5d 100644
--- a/java/jakarta/el/ImportHandler.java
+++ b/java/jakarta/el/ImportHandler.java
@@ -54,6 +54,7 @@ public class ImportHandler {
servletClassNames.add("RequestDispatcher");
servletClassNames.add("Servlet");
servletClassNames.add("ServletConfig");
+servletClassNames.add("ServletConnection");
servletClassNames.add("ServletContainerInitializer");
servletClassNames.add("ServletContext");
servletClassNames.add("ServletContextAttributeListener");
diff --git a/java/jakarta/servlet/ServletConnection.java
b/java/jakarta/servlet/ServletConnection.java
new file mode 100644
index 000..97ded16
--- /dev/null
+++ b/java/jakarta/servlet/ServletConnection.java
@@ -0,0 +1,95 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one or more
+* contributor license agreements. See the NOTICE file distributed with
+* this work for additional information regarding copyright ownership.
+* The ASF licenses this file to You under the Apache License, Version 2.0
+* (the "License"); you may not use this file except in compliance with
+* the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package jakarta.servlet;
+
+/**
+ * Provides information about the connection made to the Servlet container.
This
+ * interface is intended primarily for debugging purposes and as such provides
+ * the raw information as seen by the container. Unless explicitly stated
+ * otherwise in the Javadoc for a method, no adjustment is made for the
presence
+ * of reverse proxies or similar configurations.
+ *
+ * @since Servlet 6.0
+ */
+public interface ServletConnection {
+
+/**
+ * Obtain a unique (within the lifetime of the JVM) identifier string for
+ * the network connection to the JVM that is being used for the
+ * {@code ServletRequest} from which this {@code ServletConnection} was
+ * obtained.
+ *
+ * There is no defined format for this string. The format is implementation
+
Re: [tomcat] branch main updated: Implement the new connection ID and request ID API for Servlet 6.0
On 24/09/2021 18:30, ma...@apache.org wrote: This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git The following commit(s) were added to refs/heads/main by this push: new b7c05a8 Implement the new connection ID and request ID API for Servlet 6.0 Doing some simple testing locally highlighted something interesting. The Coyote Request gets recycled twice for most requests. Once when the HTTP request is complete and once when the Processor is recycled just before the socket is added to the Poller. I took a look to see if I could convince myself that one of these could be removed. The first is definitely required for pipelined requests. The second looks to be required for some error conditions. If anyone fancies a brain teaser then finding a way to have just one call to Request.recycle() might keep you entertained for a while ;) Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[tomcat] branch main updated: Implement the new connection ID and request ID API for Servlet 6.0
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new b7c05a8 Implement the new connection ID and request ID API for
Servlet 6.0
b7c05a8 is described below
commit b7c05a8f60003c42e6f367bf307188ce391dbad2
Author: Mark Thomas
AuthorDate: Fri Sep 24 18:30:24 2021 +0100
Implement the new connection ID and request ID API for Servlet 6.0
---
java/jakarta/el/ImportHandler.java | 1 +
java/jakarta/servlet/ServletConnection.java| 95 +++
java/jakarta/servlet/ServletRequest.java | 48
java/jakarta/servlet/ServletRequestWrapper.java| 36 ++
java/org/apache/catalina/Globals.java | 16 ---
java/org/apache/catalina/connector/Request.java| 48
.../apache/catalina/connector/RequestFacade.java | 19 +++
java/org/apache/coyote/AbstractProcessor.java | 37 +++---
java/org/apache/coyote/ActionCode.java | 13 ++-
java/org/apache/coyote/Request.java| 39 ++-
java/org/apache/coyote/ajp/AjpProcessor.java | 7 ++
java/org/apache/coyote/http11/Http11Processor.java | 7 ++
.../coyote/http2/Http2AsyncUpgradeHandler.java | 4 +-
java/org/apache/coyote/http2/Http2Protocol.java| 4 +-
.../apache/coyote/http2/Http2UpgradeHandler.java | 20 +++-
java/org/apache/coyote/http2/StreamProcessor.java | 18 +--
.../tomcat/util/net/ServletConnectionImpl.java | 55 +
.../apache/tomcat/util/net/SocketWrapperBase.java | 30 +
.../catalina/filters/TesterHttpServletRequest.java | 16 +++
.../apache/coyote/http2/TestAbstractStream.java| 130 +++--
webapps/docs/changelog.xml | 4 +
21 files changed, 553 insertions(+), 94 deletions(-)
diff --git a/java/jakarta/el/ImportHandler.java
b/java/jakarta/el/ImportHandler.java
index 138a6da..b824d5d 100644
--- a/java/jakarta/el/ImportHandler.java
+++ b/java/jakarta/el/ImportHandler.java
@@ -54,6 +54,7 @@ public class ImportHandler {
servletClassNames.add("RequestDispatcher");
servletClassNames.add("Servlet");
servletClassNames.add("ServletConfig");
+servletClassNames.add("ServletConnection");
servletClassNames.add("ServletContainerInitializer");
servletClassNames.add("ServletContext");
servletClassNames.add("ServletContextAttributeListener");
diff --git a/java/jakarta/servlet/ServletConnection.java
b/java/jakarta/servlet/ServletConnection.java
new file mode 100644
index 000..97ded16
--- /dev/null
+++ b/java/jakarta/servlet/ServletConnection.java
@@ -0,0 +1,95 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one or more
+* contributor license agreements. See the NOTICE file distributed with
+* this work for additional information regarding copyright ownership.
+* The ASF licenses this file to You under the Apache License, Version 2.0
+* (the "License"); you may not use this file except in compliance with
+* the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+package jakarta.servlet;
+
+/**
+ * Provides information about the connection made to the Servlet container.
This
+ * interface is intended primarily for debugging purposes and as such provides
+ * the raw information as seen by the container. Unless explicitly stated
+ * otherwise in the Javadoc for a method, no adjustment is made for the
presence
+ * of reverse proxies or similar configurations.
+ *
+ * @since Servlet 6.0
+ */
+public interface ServletConnection {
+
+/**
+ * Obtain a unique (within the lifetime of the JVM) identifier string for
+ * the network connection to the JVM that is being used for the
+ * {@code ServletRequest} from which this {@code ServletConnection} was
+ * obtained.
+ *
+ * There is no defined format for this string. The format is implementation
+ * dependent.
+ *
+ * @return A unique identifier for the network connection
+ */
+String getConnectionId();
+
+/**
+ * Obtain the name of the protocol as presented to the server after the
+ * removal, if present, of any TLS or similar encryption. This may not be
+ * the same as the protocol seen by the application. For example, a reverse
+ * proxy may present AJP whereas the application will see HTTP 1.1.
+ *
+ * If the protocol has an entry in the https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype
