This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new 1cf262375b Improve resource clean-up during error handling for
WebSocket
1cf262375b is described below
commit 1cf262375bca6d15d63d041db9c472431ca82471
Author: Mark Thomas <[email protected]>
AuthorDate: Mon Oct 26 17:14:25 2026 +0000
Improve resource clean-up during error handling for WebSocket
---
.../tomcat/websocket/LocalStrings.properties | 3 +
.../tomcat/websocket/WsRemoteEndpointImplBase.java | 58 ++++++++----
java/org/apache/tomcat/websocket/WsSession.java | 100 ++++++++++++++++++++-
.../tomcat/websocket/WsWebSocketContainer.java | 29 +++++-
.../tomcat/websocket/pojo/PojoEndpointBase.java | 6 +-
.../websocket/server/LocalStrings.properties | 2 +
.../websocket/server/WsHttpUpgradeHandler.java | 40 +++++++--
.../web/tomcat/OpenWebBeansInstanceManager.java | 2 +-
webapps/docs/changelog.xml | 3 +
9 files changed, 215 insertions(+), 28 deletions(-)
diff --git a/java/org/apache/tomcat/websocket/LocalStrings.properties
b/java/org/apache/tomcat/websocket/LocalStrings.properties
index 6c4e22f25a..350ce22aee 100644
--- a/java/org/apache/tomcat/websocket/LocalStrings.properties
+++ b/java/org/apache/tomcat/websocket/LocalStrings.properties
@@ -117,7 +117,9 @@ wsRemoteEndpoint.writeTimeout=Blocking write timeout
wsRemoteEndpoint.wrongState=The remote endpoint was in state [{0}] which is an
invalid state for called method
wsSession.closed=The WebSocket session [{0}] has been closed and no method
(apart from close()) may be called on a closed session
+wsSession.closeImmediately=Closing WebSocket session [{0}] without waiting for
the closing handshake to complete
wsSession.created=Created WebSocket session [{0}]
+wsSession.destroyEncodersFailed=Failed to destroy the encoders of a WebSocket
session that could not be created
wsSession.doClose=Closing WebSocket session [{0}]
wsSession.duplicateHandlerBinary=A binary message handler has already been
configured
wsSession.duplicateHandlerPong=A pong message handler has already been
configured
@@ -141,6 +143,7 @@ wsWebSocketContainer.asynchronousSocketChannelFail=Unable
to open a connection t
wsWebSocketContainer.closeSessionFail=Failed to close WebSocket session during
error handling
wsWebSocketContainer.connect.entry=Connecting endpoint instance of type [{0}]
to [{1}]
wsWebSocketContainer.connect.write=Writing the HTTP upgrade request from
buffer starting at [{0}] with a limit of [{1}] from local address [{2}]
+wsWebSocketContainer.connectAborted=The WebSocket connection could not be
established
wsWebSocketContainer.defaultConfiguratorFail=Failed to create the default
configurator
wsWebSocketContainer.failedAuthentication=Failed to handle HTTP response code
[{0}]. [{1}] header was not accepted by server.
wsWebSocketContainer.httpRequestFailed=The HTTP request to initiate the
WebSocket connection to [{0}] failed
diff --git a/java/org/apache/tomcat/websocket/WsRemoteEndpointImplBase.java
b/java/org/apache/tomcat/websocket/WsRemoteEndpointImplBase.java
index 20895c9e8b..5e287038d3 100644
--- a/java/org/apache/tomcat/websocket/WsRemoteEndpointImplBase.java
+++ b/java/org/apache/tomcat/websocket/WsRemoteEndpointImplBase.java
@@ -825,22 +825,30 @@ public abstract class WsRemoteEndpointImplBase implements
RemoteEndpoint {
*/
protected void setEncoders(EndpointConfig endpointConfig) throws
DeploymentException {
encoderEntries.clear();
- for (Class<? extends Encoder> encoderClazz :
endpointConfig.getEncoders()) {
- Encoder instance;
- InstanceManager instanceManager = wsSession.getInstanceManager();
- try {
- if (instanceManager == null) {
- instance = encoderClazz.getConstructor().newInstance();
- } else {
- instance = (Encoder)
instanceManager.newInstance(encoderClazz);
+ try {
+ for (Class<? extends Encoder> encoderClazz :
endpointConfig.getEncoders()) {
+ Encoder instance;
+ InstanceManager instanceManager =
wsSession.getInstanceManager();
+ try {
+ if (instanceManager == null) {
+ instance = encoderClazz.getConstructor().newInstance();
+ } else {
+ instance = (Encoder)
instanceManager.newInstance(encoderClazz);
+ }
+ // Add the entry before initialising the encoder below so
that the encoder is released by the
+ // clean-up below if the initialisation fails.
+ encoderEntries.add(new
EncoderEntry(Util.getEncoderType(encoderClazz), instance));
+ instance.init(endpointConfig);
+ } catch (ReflectiveOperationException | NamingException e) {
+ throw new DeploymentException(
+ sm.getString("wsRemoteEndpoint.invalidEncoder",
encoderClazz.getName()), e);
}
- instance.init(endpointConfig);
- } catch (ReflectiveOperationException | NamingException e) {
- throw new
DeploymentException(sm.getString("wsRemoteEndpoint.invalidEncoder",
encoderClazz.getName()),
- e);
}
- EncoderEntry entry = new
EncoderEntry(Util.getEncoderType(encoderClazz), instance);
- encoderEntries.add(entry);
+ } catch (RuntimeException e) {
+ // This endpoint is not going to be used, so close() below will
never be called for it. Release the encoders
+ // created so far, since nothing else will.
+ destroyEncoders();
+ throw e;
}
}
@@ -856,9 +864,11 @@ public abstract class WsRemoteEndpointImplBase implements
RemoteEndpoint {
/**
- * Closes all encoders associated with this endpoint.
+ * Destroys the encoders associated with this endpoint, including any
clean-up the instance manager performs when an
+ * instance it created is destroyed. The encoders are only destroyed once,
no matter how many times this method is
+ * called.
*/
- public final void close() {
+ void destroyEncoders() {
InstanceManager instanceManager = wsSession.getInstanceManager();
for (EncoderEntry entry : encoderEntries) {
entry.getEncoder().destroy();
@@ -870,9 +880,23 @@ public abstract class WsRemoteEndpointImplBase implements
RemoteEndpoint {
}
}
}
+ // The entries are removed, so that a repeated call does not destroy
the encoders a second time.
+ encoderEntries.clear();
+ }
+
+
+ /**
+ * Closes all encoders associated with this endpoint.
+ */
+ public final void close() {
+ destroyEncoders();
// The transformation handles both input and output. It only needs to
be
// closed once so it is closed here on the output side.
- transformation.close();
+ if (transformation != null) {
+ // The transformation is set once the connection is fully
established. It is not set if the connection
+ // could not be established, in which case there is nothing to
close here.
+ transformation.close();
+ }
doClose();
}
diff --git a/java/org/apache/tomcat/websocket/WsSession.java
b/java/org/apache/tomcat/websocket/WsSession.java
index e8dff8d270..c35e4e2609 100644
--- a/java/org/apache/tomcat/websocket/WsSession.java
+++ b/java/org/apache/tomcat/websocket/WsSession.java
@@ -58,6 +58,7 @@ import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.InstanceManager;
import org.apache.tomcat.util.ExceptionUtils;
import org.apache.tomcat.util.res.StringManager;
+import org.apache.tomcat.websocket.pojo.PojoEndpointBase;
import org.apache.tomcat.websocket.pojo.PojoEndpointServer;
import org.apache.tomcat.websocket.server.DefaultServerEndpointConfigurator;
@@ -169,7 +170,12 @@ public class WsSession implements Session {
this.userProperties.putAll(endpointConfig.getUserProperties());
this.id = Long.toHexString(ids.getAndIncrement());
- this.localEndpoint =
clientEndpointHolder.getInstance(getInstanceManager());
+ try {
+ this.localEndpoint =
clientEndpointHolder.getInstance(getInstanceManager());
+ } catch (RuntimeException e) {
+ destroyEncodersOnFailure();
+ throw e;
+ }
if (log.isTraceEnabled()) {
log.trace(sm.getString("wsSession.created", id));
@@ -253,7 +259,11 @@ public class WsSession implements Session {
endpointInstance = instanceManager.newInstance(clazz);
}
} catch (ReflectiveOperationException | NamingException e) {
+ destroyEncodersOnFailure();
throw new
DeploymentException(sm.getString("wsSession.instanceCreateFailed"), e);
+ } catch (RuntimeException e) {
+ destroyEncodersOnFailure();
+ throw e;
}
if (endpointInstance instanceof Endpoint) {
@@ -276,6 +286,23 @@ public class WsSession implements Session {
}
+ /*
+ * Releases the encoders the remote endpoint created for this session when
the session could not be created. Since
+ * the session does not exist, close() is never called for the remote
endpoint and nothing else releases those
+ * encoders - including any clean-up the instance manager performs when it
destroys an instance it registered.
+ * Failures during the clean-up are logged rather than propagated since
they must not replace the exception that
+ * caused the session creation to fail.
+ */
+ private void destroyEncodersOnFailure() {
+ try {
+ wsRemoteEndpoint.destroyEncoders();
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
+ log.warn(sm.getString("wsSession.destroyEncodersFailed"), t);
+ }
+ }
+
+
/**
* Returns the instance manager for this session.
*
@@ -628,6 +655,55 @@ public class WsSession implements Session {
}
+ /**
+ * Closes the session without waiting for the remote endpoint to complete
the closing handshake. This is used when
+ * the connection could not be established, for example because the end
point's onOpen method threw an exception.
+ * Such a session is not registered with the container so, unlike other
sessions that are waiting for the closing
+ * handshake to complete, the session close timeout processing will never
see this session. The close process has to
+ * be completed here instead, otherwise the resources associated with the
session are never released - notably the
+ * end point instance and the encoders held by the instance manager.
+ * <p>
+ * If the close process has already been started, for example because the
end point closed the session from its
+ * onOpen method, the close reason provided here is ignored and the close
process that was started is completed.
+ *
+ * @param closeReason The close reason to use if the close process has not
yet been started
+ */
+ public void closeImmediately(CloseReason closeReason) {
+ try {
+ // Start the close process if it has not already been started.
Note: the socket is closed rather than
+ // waiting for the remote endpoint to respond to the close message
that is sent.
+ doClose(closeReason, closeReason, true);
+ } catch (RuntimeException e) {
+ /*
+ * The close message could not be sent. This happens if the
connection was never fully established, for
+ * example if the frame used to process the WebSocket protocol
could not be created. Complete the close
+ * here without sending the close message. The resources
associated with the session still need to be
+ * released - notably the end point instance and the encoders held
by the instance manager.
+ */
+ if (log.isWarnEnabled()) {
+ log.warn(sm.getString("wsSession.sendCloseFail", id), e);
+ }
+ if (state.compareAndSet(State.OPEN, State.CLOSED) ||
+ state.compareAndSet(State.OUTPUT_CLOSING, State.CLOSED)) {
+ fireEndpointOnClose(closeReason);
+ closeConnection();
+ }
+ }
+
+ // If the close process was started elsewhere, the call above is
ignored since the close process only runs once.
+ // Complete that process here. Note: the CLOSING state is not handled
below since that state is only reached
+ // once a close message has been received from the remote endpoint
which, for a session that was never fully
+ // established, can not have happened.
+ if (state.compareAndSet(State.OUTPUT_CLOSING, State.CLOSED) ||
+ state.compareAndSet(State.OUTPUT_CLOSED, State.CLOSED)) {
+ if (log.isTraceEnabled()) {
+ log.trace(sm.getString("wsSession.closeImmediately", id));
+ }
+ closeConnection();
+ }
+ }
+
+
/**
* Called when a close message is received. Should only ever happen once.
Also called after a protocol error when
* the ProtocolHandler needs to force the closing of the connection.
@@ -756,7 +832,7 @@ public class WsSession implements Session {
} finally {
if (instanceManager != null) {
try {
- instanceManager.destroyInstance(localEndpoint);
+
instanceManager.destroyInstance(getManagedEndpointInstance());
} catch (Throwable t2) {
ExceptionUtils.handleThrowable(t2);
if (throwable == null) {
@@ -773,6 +849,26 @@ public class WsSession implements Session {
}
+ /*
+ * The instance the instance manager registered is the one it created or,
for an endpoint instance the application
+ * provides itself (via a configurator on the server or when connecting a
client), the one passed to
+ * InstanceManager.newInstance(Object). Annotated POJO endpoints are
wrapped by a PojoEndpointBase implementation
+ * before use, so the wrapper is not an instance the instance manager
knows about and passing it to
+ * destroyInstance(Object) would by-pass the clean-up the instance manager
performs when it destroys an instance it
+ * registered. Return the registered instance, which is the wrapper for a
standard Endpoint and the POJO for a POJO
+ * endpoint.
+ */
+ private Object getManagedEndpointInstance() {
+ if (localEndpoint instanceof PojoEndpointBase pojoEndpoint) {
+ Object pojo = pojoEndpoint.getPojo();
+ if (pojo != null) {
+ return pojo;
+ }
+ }
+ return localEndpoint;
+ }
+
+
private void fireEndpointOnError(Throwable throwable) {
// Fire the onError event
diff --git a/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
b/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
index 5d0bd10587..8cdbecded3 100644
--- a/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
+++ b/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
@@ -507,7 +507,20 @@ public class WsWebSocketContainer implements
WebSocketContainer, BackgroundProce
WsSession wsSession = new WsSession(clientEndpointHolder,
wsRemoteEndpointClient, this, extensionsAgreed,
subProtocol, Collections.emptyMap(), secure,
clientEndpointConfiguration);
- WsFrameClient wsFrameClient = new WsFrameClient(response, channel,
wsSession, transformation);
+ WsFrameClient wsFrameClient;
+ try {
+ wsFrameClient = new WsFrameClient(response, channel, wsSession,
transformation);
+ } catch (RuntimeException e) {
+ /*
+ * The session has been created, including the end point instance
obtained from the instance manager, but
+ * the connection can not be used. The session is not registered
with the container, so the session close
+ * timeout processing will never see it. Close the session here,
so that the instance manager releases the
+ * end point instance and the encoders.
+ */
+ closeSessionAfterError(wsSession,
+ new CloseReason(CloseCodes.CLOSED_ABNORMALLY,
sm.getString("wsWebSocketContainer.connectAborted")));
+ throw e;
+ }
// WsFrame adds the necessary final transformations. Copy the
// completed transformation chain to the remote end point.
wsRemoteEndpointClient.setTransformation(wsFrameClient.getTransformation());
@@ -543,6 +556,20 @@ public class WsWebSocketContainer implements
WebSocketContainer, BackgroundProce
}
+ /*
+ * Closes the session when the connection could not be established.
Failures during the close are logged rather than
+ * propagated since they must not replace the exception that is being
propagated.
+ */
+ private void closeSessionAfterError(WsSession wsSession, CloseReason
closeReason) {
+ try {
+ wsSession.closeImmediately(closeReason);
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
+ log.warn(sm.getString("wsWebSocketContainer.closeSessionFail"), t);
+ }
+ }
+
+
private Session processAuthenticationChallenge(ClientEndpointHolder
clientEndpointHolder,
ClientEndpointConfig clientEndpointConfiguration, URI
serverEndpointUri, Set<URI> redirectSet,
Map<String,Object> userProperties, String authenticationMethod,
String authenticationUri,
diff --git a/java/org/apache/tomcat/websocket/pojo/PojoEndpointBase.java
b/java/org/apache/tomcat/websocket/pojo/PojoEndpointBase.java
index 0ba88399e2..e2c904ee61 100644
--- a/java/org/apache/tomcat/websocket/pojo/PojoEndpointBase.java
+++ b/java/org/apache/tomcat/websocket/pojo/PojoEndpointBase.java
@@ -146,11 +146,13 @@ public abstract class PojoEndpointBase extends Endpoint {
}
/**
- * Returns the POJO instance wrapped by this endpoint.
+ * Returns the POJO instance wrapped by this endpoint. This is the
instance that the container's instance manager
+ * created (or registered) so, when the session using this endpoint
closes, it is this instance and not the wrapper
+ * that must be passed to the instance manager to destroy it.
*
* @return the POJO instance
*/
- protected Object getPojo() {
+ public Object getPojo() {
return pojo;
}
diff --git a/java/org/apache/tomcat/websocket/server/LocalStrings.properties
b/java/org/apache/tomcat/websocket/server/LocalStrings.properties
index 6a40e51bb2..b75722c953 100644
--- a/java/org/apache/tomcat/websocket/server/LocalStrings.properties
+++ b/java/org/apache/tomcat/websocket/server/LocalStrings.properties
@@ -38,6 +38,8 @@ wsFrameServer.onDataAvailable=Method entry
wsHttpUpgradeHandler.closeOnError=Closing WebSocket connection due to an error
wsHttpUpgradeHandler.closeSessionFail=Failed to close WebSocket session during
error handling
wsHttpUpgradeHandler.destroyFailed=Failed to close WebConnection while
destroying the WebSocket HttpUpgradeHandler
+wsHttpUpgradeHandler.errorHandlerFail=Failed to call the WebSocket endpoint
error handler during error handling
+wsHttpUpgradeHandler.initFailed=The WebSocket connection could not be
established
wsHttpUpgradeHandler.noPreInit=The preInit() method must be called to
configure the WebSocket HttpUpgradeHandler before the container calls init().
Usually, this means the Servlet that created the WsHttpUpgradeHandler instance
should also call preInit()
wsHttpUpgradeHandler.serverStop=The server is stopping
diff --git a/java/org/apache/tomcat/websocket/server/WsHttpUpgradeHandler.java
b/java/org/apache/tomcat/websocket/server/WsHttpUpgradeHandler.java
index 28205688a9..6a3a3a43c0 100644
--- a/java/org/apache/tomcat/websocket/server/WsHttpUpgradeHandler.java
+++ b/java/org/apache/tomcat/websocket/server/WsHttpUpgradeHandler.java
@@ -149,24 +149,54 @@ public class WsHttpUpgradeHandler implements
InternalHttpUpgradeHandler {
} catch (Throwable t) {
// If this really is fatal re-thrown
ExceptionUtils.handleThrowable(t);
- ep.onError(wsSession, t);
try {
- CloseReason cr = new
CloseReason(CloseCodes.CLOSED_ABNORMALLY, t.getMessage());
- wsSession.close(cr);
- } catch (IOException ioe) {
-
log.warn(sm.getString("wsHttpUpgradeHandler.closeSessionFail"), ioe);
+ ep.onError(wsSession, t);
+ } catch (Throwable t2) {
+ // If this really is fatal re-thrown
+ ExceptionUtils.handleThrowable(t2);
+
log.warn(sm.getString("wsHttpUpgradeHandler.errorHandlerFail"), t2);
}
+ closeSessionAfterError(new
CloseReason(CloseCodes.CLOSED_ABNORMALLY, t.getMessage()));
throw new IllegalArgumentException(t);
}
webSocketContainer.registerSession(serverEndpointConfig.getPath(),
wsSession, session);
} catch (DeploymentException e) {
throw new IllegalArgumentException(e);
+ } catch (RuntimeException e) {
+ /*
+ * Something went wrong after the session was created. The session
is not registered with the container, so
+ * the session close timeout processing will never see it and
destroy() below does not use it. Close the
+ * session here, so that the instance manager releases the end
point instance and the encoders. Note: the
+ * session may already be closing or closed, for example if the
end point's onOpen method threw the
+ * exception handled above, in which case this completes that
close process rather than starting a new one.
+ */
+ closeSessionAfterError(
+ new CloseReason(CloseCodes.CLOSED_ABNORMALLY,
sm.getString("wsHttpUpgradeHandler.initFailed")));
+ throw e;
} finally {
currentThread.setContextClassLoader(cl);
}
}
+ /*
+ * Closes the session, if it was created, when the connection could not be
established. Failures during the close
+ * are logged rather than propagated since they must not replace the
exception that is being propagated.
+ */
+ private void closeSessionAfterError(CloseReason closeReason) {
+ if (wsSession == null) {
+ // The session was never created, so there is nothing to close.
+ return;
+ }
+ try {
+ wsSession.closeImmediately(closeReason);
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
+ log.warn(sm.getString("wsHttpUpgradeHandler.closeSessionFail"), t);
+ }
+ }
+
+
@Override
public UpgradeInfo getUpgradeInfo() {
return upgradeInfo;
diff --git
a/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java
b/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java
index d1b39d7704..a45046477e 100644
---
a/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java
+++
b/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java
@@ -62,7 +62,7 @@ public class OpenWebBeansInstanceManager implements
InstanceManager {
@Override
public void destroyInstance(Object object)
throws IllegalAccessException, InvocationTargetException {
- Instance injectorInstance = instances.get(object);
+ Instance injectorInstance = instances.remove(object);
if (injectorInstance != null) {
try {
ClassLoader oldLoader =
Thread.currentThread().getContextClassLoader();
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 399ca9f643..b6c6589180 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -704,6 +704,9 @@
contracts. This map backs the WebSocket handshake request and response
header maps. Pull request <pr>1075</pr> provided by breken-ai. (markt)
</fix>
+ <fix>
+ Improve resource clean-up during WebSocket error handling. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Web applications">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
