This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 2fb7b7e22f Improve resource clean-up during error handling for
WebSocket
2fb7b7e22f is described below
commit 2fb7b7e22fac85cca61d764185575137d6342d7e
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 eecc804aaf..537e954c8d 100644
--- a/java/org/apache/tomcat/websocket/LocalStrings.properties
+++ b/java/org/apache/tomcat/websocket/LocalStrings.properties
@@ -119,7 +119,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
@@ -143,6 +145,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 f18b61927d..632a8420b6 100644
--- a/java/org/apache/tomcat/websocket/WsRemoteEndpointImplBase.java
+++ b/java/org/apache/tomcat/websocket/WsRemoteEndpointImplBase.java
@@ -824,22 +824,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;
}
}
@@ -855,9 +863,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();
@@ -869,9 +879,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 6f83b6ce1a..148e7ef640 100644
--- a/java/org/apache/tomcat/websocket/WsSession.java
+++ b/java/org/apache/tomcat/websocket/WsSession.java
@@ -56,6 +56,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;
@@ -171,7 +172,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));
@@ -263,7 +269,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) {
@@ -286,6 +296,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);
+ }
+ }
+
+
/**
* Creates a new WebSocket session for communication between the two
provided end points. The result of
* {@link Thread#getContextClassLoader()} at the time this constructor is
called will be used when calling
@@ -722,6 +749,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.
@@ -850,7 +926,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) {
@@ -867,6 +943,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 e04ab5b22b..1603b60b01 100644
--- a/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
+++ b/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
@@ -505,7 +505,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());
@@ -541,6 +554,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 a18d0c12ee..3bd70abff8 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 08e0c6078d..883304a6d8 100644
--- a/java/org/apache/tomcat/websocket/server/LocalStrings.properties
+++ b/java/org/apache/tomcat/websocket/server/LocalStrings.properties
@@ -39,6 +39,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 bbf0b2339a..1be248fbe6 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 6ccc9e3f19..ffa53e4362 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
@@ -61,7 +61,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 8e8c0fdb8c..f66d398ea1 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -693,6 +693,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]
