Merge pull request #2647 from igniterealtime/backport-2635-to-4.9

[Backport 4.9] OF-2921: Prevent deadlock by not broadcasting synchronously
igniterealtime · Jan 4, 2025 · 2214420 · 2214420
2 parents 398ec51 + e83dd82
commit 2214420
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 18 deletions.
diff --git a/xmppserver/src/main/java/org/jivesoftware/openfire/SessionManager.java b/xmppserver/src/main/java/org/jivesoftware/openfire/SessionManager.java
@@ -291,7 +291,9 @@ public synchronized void terminateDetached(LocalSession session) {
                     Presence presence = new Presence();
                     presence.setType(Presence.Type.unavailable);
                     presence.setFrom(session.getAddress());
-                    router.route(presence);
+
+                    // Broadcast asynchronously, to reduce the likelihood of the broadcast introducing a deadlock (OF-2921).
+                    TaskEngine.getInstance().submit(() -> router.route(presence));
                 }
 
                 session.getStreamManager().onClose(router, serverAddress);
@@ -1360,7 +1362,9 @@ public void onConnectionClose(Object handback) {
                         Presence presence = new Presence();
                         presence.setType(Presence.Type.unavailable);
                         presence.setFrom(session.getAddress());
-                        router.route(presence);
+
+                        // Broadcast asynchronously, to reduce the likelihood of the broadcast introducing a deadlock (OF-2921).
+                        TaskEngine.getInstance().submit(() -> router.route(presence));
                     }
 
                     session.getStreamManager().onClose(router, serverAddress);

diff --git a/xmppserver/src/main/java/org/jivesoftware/openfire/session/LocalClientSession.java b/xmppserver/src/main/java/org/jivesoftware/openfire/session/LocalClientSession.java
@@ -949,14 +949,25 @@ public void deliver(Packet queueOrPushStanza) throws UnauthorizedException {
         if (stanzasToPush.isEmpty()) {
             return;
         }
-        synchronized (streamManager)
-        {
+
+        // When stream management is enabled, deliver and record stanzas under a mutex. If it's not enabled, don't
+        // acquire the lock to reduce lock contention (OF-2921).
+        if (!streamManager.isEnabled()) {
             // Push stanzas to the client.
             for (final Packet stanzaToPush : stanzasToPush) {
                 if (conn != null) {
                     conn.deliver(stanzaToPush);
                 }
-                streamManager.sentStanza(stanzaToPush);
+            }
+        } else {
+            synchronized (streamManager) {
+                // Push stanzas to the client.
+                for (final Packet stanzaToPush : stanzasToPush) {
+                    if (conn != null) {
+                        conn.deliver(stanzaToPush);
+                    }
+                    streamManager.sentStanza(stanzaToPush);
+                }
             }
         }
     }

diff --git a/...r/src/main/java/org/jivesoftware/openfire/websocket/WebSocketClientConnectionHandler.java b/...r/src/main/java/org/jivesoftware/openfire/websocket/WebSocketClientConnectionHandler.java
@@ -170,20 +170,18 @@ public void onTextMethod(String stanza)
     public void onError(Throwable error)
     {
         Log.debug("Error detected; connection: {}, session: {}", wsConnection, wsSession, error);
-        synchronized (this) {
-            try {
-                if (isWebSocketOpen()) {
-                    Log.warn("Attempting to close connection on which an error occurred: {}", wsConnection, error);
-                    wsConnection.close(new StreamError(StreamError.Condition.internal_server_error), !isWebSocketOpen());
-                } else {
-                    Log.debug("Error detected on websocket that isn't open (any more):", error);
-                    wsConnection.close(null, !isWebSocketOpen());
-                }
-            } catch (Exception e) {
-                Log.error("Error disconnecting websocket", e);
-            } finally {
-                wsSession = null;
+        try {
+            if (isWebSocketOpen()) {
+                Log.warn("Attempting to close connection on which an error occurred: {}", wsConnection, error);
+                wsConnection.close(new StreamError(StreamError.Condition.internal_server_error), !isWebSocketOpen());
+            } else {
+                Log.debug("Error detected on websocket that isn't open (any more):", error);
+                wsConnection.close(null, !isWebSocketOpen());
             }
+        } catch (Exception e) {
+            Log.error("Error disconnecting websocket", e);
+        } finally {
+            wsSession = null;
         }
     }