You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2020/10/09 16:43:25 UTC
[tomcat] branch master updated: Partial fix for BZ 63362 provide h2
request statistics
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push:
new 62a684c Partial fix for BZ 63362 provide h2 request statistics
62a684c is described below
commit 62a684cd2b2039a68e74813cf4e20fc95a2cebe4
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Wed Jun 24 19:24:17 2020 +0100
Partial fix for BZ 63362 provide h2 request statistics
---
java/org/apache/coyote/AbstractProtocol.java | 35 +++++++++++++++----
java/org/apache/coyote/LocalStrings.properties | 1 +
java/org/apache/coyote/UpgradeProtocol.java | 22 ++++++++++++
java/org/apache/coyote/http2/Http2Protocol.java | 41 +++++++++++++++++++++++
java/org/apache/coyote/http2/StreamProcessor.java | 6 ++++
webapps/docs/changelog.xml | 4 +++
6 files changed, 102 insertions(+), 7 deletions(-)
diff --git a/java/org/apache/coyote/AbstractProtocol.java b/java/org/apache/coyote/AbstractProtocol.java
index e214f80..159531e 100644
--- a/java/org/apache/coyote/AbstractProtocol.java
+++ b/java/org/apache/coyote/AbstractProtocol.java
@@ -69,12 +69,6 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
/**
- * Name of MBean for the Global Request Processor.
- */
- protected ObjectName rgOname = null;
-
-
- /**
* Unique ID for this connector. Only used if the connector is configured
* to use a random port as the port will change if stop(), start() is
* called.
@@ -145,6 +139,14 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
// ------------------------------- Properties managed by the ProtocolHandler
/**
+ * Name of MBean for the Global Request Processor.
+ */
+ protected ObjectName rgOname = null;
+ public ObjectName getGlobalRequestProcessorMBeanName() {
+ return rgOname;
+ }
+
+ /**
* The adapter provides the link between the ProtocolHandler and the
* connector.
*/
@@ -546,7 +548,8 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
}
if (this.domain != null) {
- rgOname = new ObjectName(domain + ":type=GlobalRequestProcessor,name=" + getName());
+ ObjectName rgOname = new ObjectName(domain + ":type=GlobalRequestProcessor,name=" + getName());
+ this.rgOname = rgOname;
Registry.getRegistry(null, null).registerComponent(
getHandler().getGlobal(), rgOname, null);
}
@@ -556,6 +559,13 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
endpoint.setDomain(domain);
endpoint.init();
+
+ UpgradeProtocol[] upgradeProtocols = findUpgradeProtocols();
+ for (UpgradeProtocol upgradeProtocol : upgradeProtocols) {
+ // Implementation note: Failure of one upgrade protocol fails the
+ // whole Connector
+ upgradeProtocol.init();
+ }
}
@@ -669,6 +679,16 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
logPortOffset();
}
+ UpgradeProtocol[] upgradeProtocols = findUpgradeProtocols();
+ for (UpgradeProtocol upgradeProtocol : upgradeProtocols) {
+ try {
+ upgradeProtocol.destroy();
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
+ getLog().error(sm.getString("abstractProtocol.upgradeProtocolDestroyError"), t);
+ }
+ }
+
try {
endpoint.destroy();
} finally {
@@ -686,6 +706,7 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
}
}
+ ObjectName rgOname = getGlobalRequestProcessorMBeanName();
if (rgOname != null) {
Registry.getRegistry(null, null).unregisterComponent(rgOname);
}
diff --git a/java/org/apache/coyote/LocalStrings.properties b/java/org/apache/coyote/LocalStrings.properties
index 83960cb..43c8d64 100644
--- a/java/org/apache/coyote/LocalStrings.properties
+++ b/java/org/apache/coyote/LocalStrings.properties
@@ -36,6 +36,7 @@ abstractProcessor.socket.ssl=Exception getting SSL attributes
abstractProtocol.mbeanDeregistrationFailed=Failed to deregister MBean named [{0}] from MBean server [{1}]
abstractProtocol.processorRegisterError=Error registering request processor
abstractProtocol.processorUnregisterError=Error unregistering request processor
+abstractProtocol.upgradeProtocolDestroyError=Error destroying upgrade protocol
abstractProtocol.waitingProcessor.add=Added processor [{0}] to waiting processors
abstractProtocol.waitingProcessor.remove=Removed processor [{0}] from waiting processors
diff --git a/java/org/apache/coyote/UpgradeProtocol.java b/java/org/apache/coyote/UpgradeProtocol.java
index dc840df..cd2767b 100644
--- a/java/org/apache/coyote/UpgradeProtocol.java
+++ b/java/org/apache/coyote/UpgradeProtocol.java
@@ -107,4 +107,26 @@ public interface UpgradeProtocol {
public default void setHttp11Protocol(AbstractProtocol<?> protocol) {
// NO-OP
}
+
+
+ /**
+ * Initialise the upgrade protocol. Called once the parent HTTP/1.1 protocol
+ * has initialised.
+ *
+ * @throws Exception If initialisation fails
+ */
+ public default void init() throws Exception {
+ // NO-OP
+ }
+
+
+ /**
+ * Destroy the upgrade protocol. Called before the parent HTTP/1.1 protocol
+ * is destroyed.
+ *
+ * @throws Exception If the upgrade protocol is not destroyed cleanly
+ */
+ public default void destroy() throws Exception {
+ // NO-OP
+ }
}
diff --git a/java/org/apache/coyote/http2/Http2Protocol.java b/java/org/apache/coyote/http2/Http2Protocol.java
index a93955b..1be95d9 100644
--- a/java/org/apache/coyote/http2/Http2Protocol.java
+++ b/java/org/apache/coyote/http2/Http2Protocol.java
@@ -19,17 +19,21 @@ package org.apache.coyote.http2;
import java.nio.charset.StandardCharsets;
import java.util.Enumeration;
+import javax.management.ObjectName;
+
import org.apache.coyote.AbstractProtocol;
import org.apache.coyote.Adapter;
import org.apache.coyote.ContinueResponseTiming;
import org.apache.coyote.Processor;
import org.apache.coyote.Request;
+import org.apache.coyote.RequestGroupInfo;
import org.apache.coyote.Response;
import org.apache.coyote.UpgradeProtocol;
import org.apache.coyote.UpgradeToken;
import org.apache.coyote.http11.AbstractHttp11Protocol;
import org.apache.coyote.http11.upgrade.InternalHttpUpgradeHandler;
import org.apache.coyote.http11.upgrade.UpgradeProcessorInternal;
+import org.apache.tomcat.util.modeler.Registry;
import org.apache.tomcat.util.net.SocketWrapperBase;
public class Http2Protocol implements UpgradeProtocol {
@@ -81,6 +85,9 @@ public class Http2Protocol implements UpgradeProtocol {
// Reference to HTTP/1.1 protocol that this instance is configured under
private AbstractHttp11Protocol<?> http11Protocol = null;
+ private RequestGroupInfo global = new RequestGroupInfo();
+ private ObjectName rgOname = null;
+
@Override
public String getHttpUpgradeName(boolean isSSLEnabled) {
if (isSSLEnabled) {
@@ -328,8 +335,42 @@ public class Http2Protocol implements UpgradeProtocol {
return this.http11Protocol;
}
+
@Override
public void setHttp11Protocol(AbstractProtocol<?> http11Protocol) {
this.http11Protocol = (AbstractHttp11Protocol<?>) http11Protocol;
}
+
+
+ public RequestGroupInfo getGlobal() {
+ return global;
+ }
+
+
+ @Override
+ public void init() throws Exception {
+ ObjectName parentRgOname = http11Protocol.getGlobalRequestProcessorMBeanName();
+ if (parentRgOname != null) {
+ StringBuilder name = new StringBuilder(parentRgOname.getCanonicalName());
+ name.append(",Upgrade=");
+ // Neither of these names need quoting
+ if (http11Protocol.isSSLEnabled()) {
+ name.append(ALPN_NAME);
+ } else {
+ name.append(HTTP_UPGRADE_NAME);
+ }
+ ObjectName rgOname = new ObjectName(name.toString());
+ this.rgOname = rgOname;
+ Registry.getRegistry(null, null).registerComponent(global, rgOname, null);
+ }
+ }
+
+
+ @Override
+ public void destroy() throws Exception {
+ ObjectName rgOname = this.rgOname;
+ if (rgOname != null) {
+ Registry.getRegistry(null, null).unregisterComponent(rgOname);
+ }
+ }
}
diff --git a/java/org/apache/coyote/http2/StreamProcessor.java b/java/org/apache/coyote/http2/StreamProcessor.java
index 94904c0..98c86cb 100644
--- a/java/org/apache/coyote/http2/StreamProcessor.java
+++ b/java/org/apache/coyote/http2/StreamProcessor.java
@@ -371,6 +371,12 @@ class StreamProcessor extends AbstractProcessor {
@Override
public final void recycle() {
// StreamProcessor instances are not re-used.
+
+ // Calling removeRequestProcessor even though the RequestProcesser was
+ // never added will add the values from the RequestProcessor to the
+ // running total for the GlobalRequestProcessor
+ handler.getProtocol().getGlobal().removeRequestProcessor(request.getRequestProcessor());
+
// Clear fields that can be cleared to aid GC and trigger NPEs if this
// is reused
setSocketWrapper(null);
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index a44879c..8450b0b 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -69,6 +69,10 @@
and the encoded solidus was preceeded by other %nn encoded characters.
Based on a pull request by willmeck. (markt)
</fix>
+ <fix>
+ Implement a partial fix for <bug>63362</bug> that adds collection of
+ request statistics for HTTP/2 requests. (markt)
+ </fix>
</changelog>
</subseciton>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch master updated: Partial fix for BZ 63362 provide
h2 request statistics
Posted by Mark Thomas <ma...@apache.org>.
On 09/10/2020 17:43, markt@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> markt pushed a commit to branch master
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/master by this push:
> new 62a684c Partial fix for BZ 63362 provide h2 request statistics
> 62a684c is described below
>
> commit 62a684cd2b2039a68e74813cf4e20fc95a2cebe4
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Wed Jun 24 19:24:17 2020 +0100
>
> Partial fix for BZ 63362 provide h2 request statistics
Just a quick heads up that as I work on WebSocket (and a generic
solution for any HTTP upgrade protocol) it is looking as if a chunk of
the changes below are going to be reverted.
Mark
> ---
> java/org/apache/coyote/AbstractProtocol.java | 35 +++++++++++++++----
> java/org/apache/coyote/LocalStrings.properties | 1 +
> java/org/apache/coyote/UpgradeProtocol.java | 22 ++++++++++++
> java/org/apache/coyote/http2/Http2Protocol.java | 41 +++++++++++++++++++++++
> java/org/apache/coyote/http2/StreamProcessor.java | 6 ++++
> webapps/docs/changelog.xml | 4 +++
> 6 files changed, 102 insertions(+), 7 deletions(-)
>
> diff --git a/java/org/apache/coyote/AbstractProtocol.java b/java/org/apache/coyote/AbstractProtocol.java
> index e214f80..159531e 100644
> --- a/java/org/apache/coyote/AbstractProtocol.java
> +++ b/java/org/apache/coyote/AbstractProtocol.java
> @@ -69,12 +69,6 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
>
>
> /**
> - * Name of MBean for the Global Request Processor.
> - */
> - protected ObjectName rgOname = null;
> -
> -
> - /**
> * Unique ID for this connector. Only used if the connector is configured
> * to use a random port as the port will change if stop(), start() is
> * called.
> @@ -145,6 +139,14 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
> // ------------------------------- Properties managed by the ProtocolHandler
>
> /**
> + * Name of MBean for the Global Request Processor.
> + */
> + protected ObjectName rgOname = null;
> + public ObjectName getGlobalRequestProcessorMBeanName() {
> + return rgOname;
> + }
> +
> + /**
> * The adapter provides the link between the ProtocolHandler and the
> * connector.
> */
> @@ -546,7 +548,8 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
> }
>
> if (this.domain != null) {
> - rgOname = new ObjectName(domain + ":type=GlobalRequestProcessor,name=" + getName());
> + ObjectName rgOname = new ObjectName(domain + ":type=GlobalRequestProcessor,name=" + getName());
> + this.rgOname = rgOname;
> Registry.getRegistry(null, null).registerComponent(
> getHandler().getGlobal(), rgOname, null);
> }
> @@ -556,6 +559,13 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
> endpoint.setDomain(domain);
>
> endpoint.init();
> +
> + UpgradeProtocol[] upgradeProtocols = findUpgradeProtocols();
> + for (UpgradeProtocol upgradeProtocol : upgradeProtocols) {
> + // Implementation note: Failure of one upgrade protocol fails the
> + // whole Connector
> + upgradeProtocol.init();
> + }
> }
>
>
> @@ -669,6 +679,16 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
> logPortOffset();
> }
>
> + UpgradeProtocol[] upgradeProtocols = findUpgradeProtocols();
> + for (UpgradeProtocol upgradeProtocol : upgradeProtocols) {
> + try {
> + upgradeProtocol.destroy();
> + } catch (Throwable t) {
> + ExceptionUtils.handleThrowable(t);
> + getLog().error(sm.getString("abstractProtocol.upgradeProtocolDestroyError"), t);
> + }
> + }
> +
> try {
> endpoint.destroy();
> } finally {
> @@ -686,6 +706,7 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler,
> }
> }
>
> + ObjectName rgOname = getGlobalRequestProcessorMBeanName();
> if (rgOname != null) {
> Registry.getRegistry(null, null).unregisterComponent(rgOname);
> }
> diff --git a/java/org/apache/coyote/LocalStrings.properties b/java/org/apache/coyote/LocalStrings.properties
> index 83960cb..43c8d64 100644
> --- a/java/org/apache/coyote/LocalStrings.properties
> +++ b/java/org/apache/coyote/LocalStrings.properties
> @@ -36,6 +36,7 @@ abstractProcessor.socket.ssl=Exception getting SSL attributes
> abstractProtocol.mbeanDeregistrationFailed=Failed to deregister MBean named [{0}] from MBean server [{1}]
> abstractProtocol.processorRegisterError=Error registering request processor
> abstractProtocol.processorUnregisterError=Error unregistering request processor
> +abstractProtocol.upgradeProtocolDestroyError=Error destroying upgrade protocol
> abstractProtocol.waitingProcessor.add=Added processor [{0}] to waiting processors
> abstractProtocol.waitingProcessor.remove=Removed processor [{0}] from waiting processors
>
> diff --git a/java/org/apache/coyote/UpgradeProtocol.java b/java/org/apache/coyote/UpgradeProtocol.java
> index dc840df..cd2767b 100644
> --- a/java/org/apache/coyote/UpgradeProtocol.java
> +++ b/java/org/apache/coyote/UpgradeProtocol.java
> @@ -107,4 +107,26 @@ public interface UpgradeProtocol {
> public default void setHttp11Protocol(AbstractProtocol<?> protocol) {
> // NO-OP
> }
> +
> +
> + /**
> + * Initialise the upgrade protocol. Called once the parent HTTP/1.1 protocol
> + * has initialised.
> + *
> + * @throws Exception If initialisation fails
> + */
> + public default void init() throws Exception {
> + // NO-OP
> + }
> +
> +
> + /**
> + * Destroy the upgrade protocol. Called before the parent HTTP/1.1 protocol
> + * is destroyed.
> + *
> + * @throws Exception If the upgrade protocol is not destroyed cleanly
> + */
> + public default void destroy() throws Exception {
> + // NO-OP
> + }
> }
> diff --git a/java/org/apache/coyote/http2/Http2Protocol.java b/java/org/apache/coyote/http2/Http2Protocol.java
> index a93955b..1be95d9 100644
> --- a/java/org/apache/coyote/http2/Http2Protocol.java
> +++ b/java/org/apache/coyote/http2/Http2Protocol.java
> @@ -19,17 +19,21 @@ package org.apache.coyote.http2;
> import java.nio.charset.StandardCharsets;
> import java.util.Enumeration;
>
> +import javax.management.ObjectName;
> +
> import org.apache.coyote.AbstractProtocol;
> import org.apache.coyote.Adapter;
> import org.apache.coyote.ContinueResponseTiming;
> import org.apache.coyote.Processor;
> import org.apache.coyote.Request;
> +import org.apache.coyote.RequestGroupInfo;
> import org.apache.coyote.Response;
> import org.apache.coyote.UpgradeProtocol;
> import org.apache.coyote.UpgradeToken;
> import org.apache.coyote.http11.AbstractHttp11Protocol;
> import org.apache.coyote.http11.upgrade.InternalHttpUpgradeHandler;
> import org.apache.coyote.http11.upgrade.UpgradeProcessorInternal;
> +import org.apache.tomcat.util.modeler.Registry;
> import org.apache.tomcat.util.net.SocketWrapperBase;
>
> public class Http2Protocol implements UpgradeProtocol {
> @@ -81,6 +85,9 @@ public class Http2Protocol implements UpgradeProtocol {
> // Reference to HTTP/1.1 protocol that this instance is configured under
> private AbstractHttp11Protocol<?> http11Protocol = null;
>
> + private RequestGroupInfo global = new RequestGroupInfo();
> + private ObjectName rgOname = null;
> +
> @Override
> public String getHttpUpgradeName(boolean isSSLEnabled) {
> if (isSSLEnabled) {
> @@ -328,8 +335,42 @@ public class Http2Protocol implements UpgradeProtocol {
> return this.http11Protocol;
> }
>
> +
> @Override
> public void setHttp11Protocol(AbstractProtocol<?> http11Protocol) {
> this.http11Protocol = (AbstractHttp11Protocol<?>) http11Protocol;
> }
> +
> +
> + public RequestGroupInfo getGlobal() {
> + return global;
> + }
> +
> +
> + @Override
> + public void init() throws Exception {
> + ObjectName parentRgOname = http11Protocol.getGlobalRequestProcessorMBeanName();
> + if (parentRgOname != null) {
> + StringBuilder name = new StringBuilder(parentRgOname.getCanonicalName());
> + name.append(",Upgrade=");
> + // Neither of these names need quoting
> + if (http11Protocol.isSSLEnabled()) {
> + name.append(ALPN_NAME);
> + } else {
> + name.append(HTTP_UPGRADE_NAME);
> + }
> + ObjectName rgOname = new ObjectName(name.toString());
> + this.rgOname = rgOname;
> + Registry.getRegistry(null, null).registerComponent(global, rgOname, null);
> + }
> + }
> +
> +
> + @Override
> + public void destroy() throws Exception {
> + ObjectName rgOname = this.rgOname;
> + if (rgOname != null) {
> + Registry.getRegistry(null, null).unregisterComponent(rgOname);
> + }
> + }
> }
> diff --git a/java/org/apache/coyote/http2/StreamProcessor.java b/java/org/apache/coyote/http2/StreamProcessor.java
> index 94904c0..98c86cb 100644
> --- a/java/org/apache/coyote/http2/StreamProcessor.java
> +++ b/java/org/apache/coyote/http2/StreamProcessor.java
> @@ -371,6 +371,12 @@ class StreamProcessor extends AbstractProcessor {
> @Override
> public final void recycle() {
> // StreamProcessor instances are not re-used.
> +
> + // Calling removeRequestProcessor even though the RequestProcesser was
> + // never added will add the values from the RequestProcessor to the
> + // running total for the GlobalRequestProcessor
> + handler.getProtocol().getGlobal().removeRequestProcessor(request.getRequestProcessor());
> +
> // Clear fields that can be cleared to aid GC and trigger NPEs if this
> // is reused
> setSocketWrapper(null);
> diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
> index a44879c..8450b0b 100644
> --- a/webapps/docs/changelog.xml
> +++ b/webapps/docs/changelog.xml
> @@ -69,6 +69,10 @@
> and the encoded solidus was preceeded by other %nn encoded characters.
> Based on a pull request by willmeck. (markt)
> </fix>
> + <fix>
> + Implement a partial fix for <bug>63362</bug> that adds collection of
> + request statistics for HTTP/2 requests. (markt)
> + </fix>
> </changelog>
> </subseciton>
> <subsection name="Jasper">
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org