From 7377893050f2f59c606b298ee4d18161f3745ebe Mon Sep 17 00:00:00 2001 From: catbref Date: Thu, 23 Jul 2020 12:12:13 +0100 Subject: [PATCH] WebSocket improvements, inc. bump Jetty to v9.4.29-20200521 Various issues in Jetty v9.4.22 (and some later versions too) cause websockets to use up all available threads. Bumped Jetty to v9.4.29 to resolve some of these issues. Changed some Qortal-side websocket code to minimize locking on websocket notifiers. Websocket messages now sent async, although the returned Futures are discarded, as it's up to the remote end to consume fast enough. Changed Controller to only request a SysTray update before synchronization if there's a chance node might change height. Similarly, Controller only requests SysTray update after synchronization if chain tip has actually changed. Both of the above together should reduce the number of messages sent out via the admin status websockets. --- pom.xml | 2 +- .../java/org/qortal/api/ApiErrorHandler.java | 3 +- src/main/java/org/qortal/api/ApiService.java | 5 ++-- .../api/websocket/ActiveChatsWebSocket.java | 5 ++-- .../api/websocket/AdminStatusWebSocket.java | 5 ++-- .../qortal/api/websocket/BlocksWebSocket.java | 5 ++-- .../api/websocket/ChatMessagesWebSocket.java | 5 ++-- .../org/qortal/controller/BlockNotifier.java | 26 ++++++++++++---- .../org/qortal/controller/ChatNotifier.java | 30 ++++++++++++++----- .../org/qortal/controller/Controller.java | 11 +++---- .../org/qortal/controller/StatusNotifier.java | 26 ++++++++++++---- 11 files changed, 85 insertions(+), 38 deletions(-) diff --git a/pom.xml b/pom.xml index a0b05ed6..cca2cce5 100644 --- a/pom.xml +++ b/pom.xml @@ -17,7 +17,7 @@ 2.5.0-fixed 2.5.0 2.29.1 - 9.4.22.v20191022 + 9.4.29.v20200521 2.12.1 UTF-8 1.7.12 diff --git a/src/main/java/org/qortal/api/ApiErrorHandler.java b/src/main/java/org/qortal/api/ApiErrorHandler.java index 38aef3c1..a6a77eed 100644 --- a/src/main/java/org/qortal/api/ApiErrorHandler.java +++ b/src/main/java/org/qortal/api/ApiErrorHandler.java @@ -3,6 +3,7 @@ package org.qortal.api; import java.io.IOException; import javax.servlet.RequestDispatcher; +import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -17,7 +18,7 @@ public class ApiErrorHandler extends ErrorHandler { private static final Logger LOGGER = LogManager.getLogger(ApiErrorHandler.class); @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException { + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { if (Settings.getInstance().isApiLoggingEnabled()) { String requestURI = request.getRequestURI(); diff --git a/src/main/java/org/qortal/api/ApiService.java b/src/main/java/org/qortal/api/ApiService.java index 52b03cac..b88edb5a 100644 --- a/src/main/java/org/qortal/api/ApiService.java +++ b/src/main/java/org/qortal/api/ApiService.java @@ -18,9 +18,9 @@ import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.rewrite.handler.RedirectPatternRule; import org.eclipse.jetty.rewrite.handler.RewriteHandler; import org.eclipse.jetty.server.CustomRequestLog; +import org.eclipse.jetty.server.DetectorConnectionFactory; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.HttpConnectionFactory; -import org.eclipse.jetty.server.OptionalSslConnectionFactory; import org.eclipse.jetty.server.RequestLog; import org.eclipse.jetty.server.RequestLogWriter; import org.eclipse.jetty.server.SecureRequestCustomizer; @@ -113,8 +113,7 @@ public class ApiService { SslConnectionFactory sslConnectionFactory = new SslConnectionFactory(sslContextFactory, HttpVersion.HTTP_1_1.asString()); ServerConnector portUnifiedConnector = new ServerConnector(this.server, - new OptionalSslConnectionFactory(sslConnectionFactory, HttpVersion.HTTP_1_1.asString()), - sslConnectionFactory, + new DetectorConnectionFactory(sslConnectionFactory), httpConnectionFactory); portUnifiedConnector.setHost(Settings.getInstance().getBindAddress()); portUnifiedConnector.setPort(Settings.getInstance().getApiPort()); diff --git a/src/main/java/org/qortal/api/websocket/ActiveChatsWebSocket.java b/src/main/java/org/qortal/api/websocket/ActiveChatsWebSocket.java index b85b7891..0025282c 100644 --- a/src/main/java/org/qortal/api/websocket/ActiveChatsWebSocket.java +++ b/src/main/java/org/qortal/api/websocket/ActiveChatsWebSocket.java @@ -6,6 +6,7 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.websocket.api.Session; +import org.eclipse.jetty.websocket.api.WebSocketException; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketClose; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketConnect; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketMessage; @@ -78,8 +79,8 @@ public class ActiveChatsWebSocket extends WebSocketServlet implements ApiWebSock return; previousOutput.set(output); - session.getRemote().sendString(output); - } catch (DataException | IOException e) { + session.getRemote().sendStringByFuture(output); + } catch (DataException | IOException | WebSocketException e) { // No output this time? } } diff --git a/src/main/java/org/qortal/api/websocket/AdminStatusWebSocket.java b/src/main/java/org/qortal/api/websocket/AdminStatusWebSocket.java index 2a957921..87fafe0e 100644 --- a/src/main/java/org/qortal/api/websocket/AdminStatusWebSocket.java +++ b/src/main/java/org/qortal/api/websocket/AdminStatusWebSocket.java @@ -5,6 +5,7 @@ import java.io.StringWriter; import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.websocket.api.Session; +import org.eclipse.jetty.websocket.api.WebSocketException; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketClose; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketConnect; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketMessage; @@ -59,8 +60,8 @@ public class AdminStatusWebSocket extends WebSocketServlet implements ApiWebSock return; previousOutput.set(output); - session.getRemote().sendString(output); - } catch (DataException | IOException e) { + session.getRemote().sendStringByFuture(output); + } catch (DataException | IOException | WebSocketException e) { // No output this time? } } diff --git a/src/main/java/org/qortal/api/websocket/BlocksWebSocket.java b/src/main/java/org/qortal/api/websocket/BlocksWebSocket.java index 398cdd33..3339f794 100644 --- a/src/main/java/org/qortal/api/websocket/BlocksWebSocket.java +++ b/src/main/java/org/qortal/api/websocket/BlocksWebSocket.java @@ -4,6 +4,7 @@ import java.io.IOException; import java.io.StringWriter; import org.eclipse.jetty.websocket.api.Session; +import org.eclipse.jetty.websocket.api.WebSocketException; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketClose; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketConnect; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketMessage; @@ -100,8 +101,8 @@ public class BlocksWebSocket extends WebSocketServlet implements ApiWebSocket { try { this.marshall(stringWriter, blockData); - session.getRemote().sendString(stringWriter.toString()); - } catch (IOException e) { + session.getRemote().sendStringByFuture(stringWriter.toString()); + } catch (IOException | WebSocketException e) { // No output this time } } diff --git a/src/main/java/org/qortal/api/websocket/ChatMessagesWebSocket.java b/src/main/java/org/qortal/api/websocket/ChatMessagesWebSocket.java index ef04b950..99825222 100644 --- a/src/main/java/org/qortal/api/websocket/ChatMessagesWebSocket.java +++ b/src/main/java/org/qortal/api/websocket/ChatMessagesWebSocket.java @@ -8,6 +8,7 @@ import java.util.List; import java.util.Map; import org.eclipse.jetty.websocket.api.Session; +import org.eclipse.jetty.websocket.api.WebSocketException; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketClose; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketConnect; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketMessage; @@ -125,8 +126,8 @@ public class ChatMessagesWebSocket extends WebSocketServlet implements ApiWebSoc try { this.marshall(stringWriter, chatMessages); - session.getRemote().sendString(stringWriter.toString()); - } catch (IOException e) { + session.getRemote().sendStringByFuture(stringWriter.toString()); + } catch (IOException | WebSocketException e) { // No output this time? } } diff --git a/src/main/java/org/qortal/controller/BlockNotifier.java b/src/main/java/org/qortal/controller/BlockNotifier.java index d4278d05..742985ae 100644 --- a/src/main/java/org/qortal/controller/BlockNotifier.java +++ b/src/main/java/org/qortal/controller/BlockNotifier.java @@ -1,5 +1,7 @@ package org.qortal.controller; +import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.Map; @@ -27,17 +29,29 @@ public class BlockNotifier { return instance; } - public synchronized void register(Session session, Listener listener) { - this.listenersBySession.put(session, listener); + public void register(Session session, Listener listener) { + synchronized (this.listenersBySession) { + this.listenersBySession.put(session, listener); + } } - public synchronized void deregister(Session session) { - this.listenersBySession.remove(session); + public void deregister(Session session) { + synchronized (this.listenersBySession) { + this.listenersBySession.remove(session); + } } - public synchronized void onNewBlock(BlockData blockData) { - for (Listener listener : this.listenersBySession.values()) + public void onNewBlock(BlockData blockData) { + for (Listener listener : getAllListeners()) listener.notify(blockData); } + private Collection getAllListeners() { + // Make a copy of listeners to both avoid concurrent modification + // and reduce synchronization time + synchronized (this.listenersBySession) { + return new ArrayList<>(this.listenersBySession.values()); + } + } + } diff --git a/src/main/java/org/qortal/controller/ChatNotifier.java b/src/main/java/org/qortal/controller/ChatNotifier.java index 57ca7cbf..61146faa 100644 --- a/src/main/java/org/qortal/controller/ChatNotifier.java +++ b/src/main/java/org/qortal/controller/ChatNotifier.java @@ -1,5 +1,7 @@ package org.qortal.controller; +import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.Map; @@ -27,22 +29,34 @@ public class ChatNotifier { return instance; } - public synchronized void register(Session session, Listener listener) { - this.listenersBySession.put(session, listener); + public void register(Session session, Listener listener) { + synchronized (this.listenersBySession) { + this.listenersBySession.put(session, listener); + } } - public synchronized void deregister(Session session) { - this.listenersBySession.remove(session); + public void deregister(Session session) { + synchronized (this.listenersBySession) { + this.listenersBySession.remove(session); + } } - public synchronized void onNewChatTransaction(ChatTransactionData chatTransactionData) { - for (Listener listener : this.listenersBySession.values()) + public void onNewChatTransaction(ChatTransactionData chatTransactionData) { + for (Listener listener : getAllListeners()) listener.notify(chatTransactionData); } - public synchronized void onGroupMembershipChange() { - for (Listener listener : this.listenersBySession.values()) + public void onGroupMembershipChange() { + for (Listener listener : getAllListeners()) listener.notify(null); } + private Collection getAllListeners() { + // Make a copy of listeners to both avoid concurrent modification + // and reduce synchronization time + synchronized (this.listenersBySession) { + return new ArrayList<>(this.listenersBySession.values()); + } + } + } diff --git a/src/main/java/org/qortal/controller/Controller.java b/src/main/java/org/qortal/controller/Controller.java index 6c9bac27..ce593bb4 100644 --- a/src/main/java/org/qortal/controller/Controller.java +++ b/src/main/java/org/qortal/controller/Controller.java @@ -537,8 +537,11 @@ public class Controller extends Thread { public SynchronizationResult actuallySynchronize(Peer peer, boolean force) throws InterruptedException { syncPercent = (this.chainTip.getHeight() * 100) / peer.getChainTipData().getLastHeight(); - isSynchronizing = true; - updateSysTray(); + // Only update SysTray if we're potentially changing height + if (syncPercent < 100) { + isSynchronizing = true; + updateSysTray(); + } BlockData priorChainTip = this.chainTip; @@ -584,7 +587,6 @@ public class Controller extends Thread { break; case OK: - requestSysTrayUpdate = true; // fall-through... case NOTHING_TO_DO: { // Update our list of inferior chain tips @@ -611,14 +613,13 @@ public class Controller extends Thread { // Reset our cache of inferior chains inferiorChainSignatures.clear(); - // Update chain-tip, notify peers, websockets, etc. + // Update chain-tip, systray, notify peers, websockets, etc. this.onNewBlock(newChainTip); } return syncResult; } finally { isSynchronizing = false; - requestSysTrayUpdate = true; } } diff --git a/src/main/java/org/qortal/controller/StatusNotifier.java b/src/main/java/org/qortal/controller/StatusNotifier.java index 02090db3..c142d91c 100644 --- a/src/main/java/org/qortal/controller/StatusNotifier.java +++ b/src/main/java/org/qortal/controller/StatusNotifier.java @@ -1,5 +1,7 @@ package org.qortal.controller; +import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.Map; @@ -26,17 +28,29 @@ public class StatusNotifier { return instance; } - public synchronized void register(Session session, Listener listener) { - this.listenersBySession.put(session, listener); + public void register(Session session, Listener listener) { + synchronized (this.listenersBySession) { + this.listenersBySession.put(session, listener); + } } - public synchronized void deregister(Session session) { - this.listenersBySession.remove(session); + public void deregister(Session session) { + synchronized (this.listenersBySession) { + this.listenersBySession.remove(session); + } } - public synchronized void onStatusChange(long now) { - for (Listener listener : this.listenersBySession.values()) + public void onStatusChange(long now) { + for (Listener listener : getAllListeners()) listener.notify(now); } + private Collection getAllListeners() { + // Make a copy of listeners to both avoid concurrent modification + // and reduce synchronization time + synchronized (this.listenersBySession) { + return new ArrayList<>(this.listenersBySession.values()); + } + } + }