On 09/10/2020 17:43, [email protected] 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 <[email protected]>
> 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: [email protected]
> For additional commands, e-mail: [email protected]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
