martin-g commented on a change in pull request #499: URL: https://github.com/apache/wicket/pull/499#discussion_r801016846
########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/message/PongMessageMessage.java ########## @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.protocol.ws.api.message; + +import java.nio.ByteBuffer; + +import org.apache.wicket.Application; +import org.apache.wicket.protocol.ws.api.registry.IKey; +import org.apache.wicket.util.lang.Args; + +/** + * A {@link IWebSocketMessage message} with Pong message data + * + * @since 6.0 + */ +public class PongMessageMessage extends AbstractClientMessage Review comment: Why double `Message` ? ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/message/PongMessageMessage.java ########## @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.protocol.ws.api.message; + +import java.nio.ByteBuffer; + +import org.apache.wicket.Application; +import org.apache.wicket.protocol.ws.api.registry.IKey; +import org.apache.wicket.util.lang.Args; + +/** + * A {@link IWebSocketMessage message} with Pong message data + * + * @since 6.0 Review comment: not really ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/res/js/wicket-websocket-setup.js.tmpl ########## @@ -4,7 +4,10 @@ if (typeof(Wicket.WebSocket.appName) === "undefined") { jQuery.extend(Wicket.WebSocket, { pageId: ${pageId}, context: '${context}', resourceName: '${resourceName}', connectionToken: '${connectionToken}', baseUrl: '${baseUrl}', contextPath: '${contextPath}', appName: '${applicationName}', - port: ${port}, securePort: ${securePort}, filterPrefix: '${filterPrefix}', sessionId: '${sessionId}' }); + port: ${port}, securePort: ${securePort}, filterPrefix: '${filterPrefix}', sessionId: '${sessionId}', + useHeartBeat: ${useHeartBeat}, reconnectOnFailure: ${reconnectOnFailure}, + heartBeatPace: ${heartBeatPace}, networkLatencyThreshold: ${networkLatencyThreshold}}); Review comment: Do we really need `networkLatencyThreshold` ? One can easily extend the `heartBeatPace` duration via the settings. ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/util/tester/TestWebSocketConnection.java ########## @@ -42,6 +43,41 @@ public TestWebSocketConnection(WebApplication application, String sessionId, IKe this.registryKey = registryKey; } + @Override + public long getLastTimeAlive() { + return 0; + } + + @Override + public boolean isAlive() { + return false; Review comment: true ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/res/js/wicket-websocket-jquery.js ########## @@ -139,6 +155,22 @@ } }, + heartbeat: function () { + clearTimeout(this.pingTimeout); + // Set a timeout in order to check ping received + this.pingTimeout = setTimeout(() => { + this.ws.close(); + // try to reconnect to server + if (Wicket.WebSocket.reconnectOnFailure) + { + Wicket.Log.debug("Trying to reconnect to server"); + Wicket.WebSocket.INSTANCE = null; + Wicket.WebSocket.reconnect = true; Review comment: can `reconnect` be passed as a parameter to `createDefaultConnection()` instead of set as `static` field ? ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/timer/AbstractHeartBeatTimer.java ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.protocol.ws.timer; + +import java.io.IOException; +import java.util.Date; +import java.util.Timer; +import java.util.TimerTask; + +import org.apache.wicket.Application; +import org.apache.wicket.protocol.ws.WebSocketSettings; +import org.apache.wicket.protocol.ws.api.IWebSocketConnection; +import org.apache.wicket.protocol.ws.concurrent.Executor; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public abstract class AbstractHeartBeatTimer { + + private static final Logger LOG = LoggerFactory.getLogger(AbstractHeartBeatTimer.class); + + protected final WebSocketSettings webSocketSettings; + + // internal heartbeat's timer. + private Timer heartBeatsTimer; + + public AbstractHeartBeatTimer(WebSocketSettings webSocketSettings) + { + this.webSocketSettings = webSocketSettings; + } + + public final void start(Application application) + { + if (isTimerEnabled() == false) + { + return; + } + + if (LOG.isInfoEnabled()) Review comment: No need of this check. The value passed to `Logger#info()` is not expensive and the loggers do the same check internally. ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/timer/HeartBeatWithReconnectTimer.java ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.protocol.ws.timer; + +import java.io.IOException; + +import org.apache.wicket.Application; +import org.apache.wicket.protocol.ws.WebSocketSettings; +import org.apache.wicket.protocol.ws.api.IWebSocketConnection; +import org.apache.wicket.protocol.ws.concurrent.Executor; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class HeartBeatWithReconnectTimer extends AbstractHeartBeatTimer { + + private static final Logger LOG = LoggerFactory.getLogger(HeartBeatWithReconnectTimer.class); + + + public HeartBeatWithReconnectTimer(WebSocketSettings webSocketSettings) + { + super(webSocketSettings); + } + + + @Override + protected boolean isTimerEnabled() { + if (webSocketSettings.isUseHeartBeat() == false) + { + if (LOG.isInfoEnabled()) + { + LOG.info("useHeartBeat is set to false. Thus we won't start heartbeat's sending thread"); + } + return false; + } + return true; + } + + + protected void sendHeartBeats(Application application) + { + final Executor heartBeatsExecutor = webSocketSettings.getHeartBeatsExecutor(); + final int maxPingRetries = webSocketSettings.getMaxPingRetries(); + for (IWebSocketConnection connection: webSocketSettings.getConnectionRegistry().getConnections(application)) + { + heartBeatsExecutor.run(() -> ping(connection, maxPingRetries)); + } + } + + private void ping(IWebSocketConnection connection, final int pingRetryCounter) + { + try + { + // we just sent a binary message + connection.sendMessage(new byte[]{10}); Review comment: let's use some special value for the PING message, e.g. `new byte[] {(byte) 'w', (byte) 'i', 'c', 'k', 'e', 't', '-', 'w', 's', '-', 'p', 'i', 'n', 'g'}` ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/test/java/org/apache/wicket/protocol/ws/util/tester/WebSocketTesterProcessorTest.java ########## @@ -59,6 +59,11 @@ protected void onOutMessage(byte[] message, int offset, int length) { messageReceived.set(true); } + + @Override + protected void onOutMessage(byte[] message) { + messageReceived.set(true); Review comment: I think this test should call only the old callback method. This method body should be empty or throw an Exception if it is called by mistake. ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/timer/HeartBeatWithReconnectTimer.java ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.protocol.ws.timer; + +import java.io.IOException; + +import org.apache.wicket.Application; +import org.apache.wicket.protocol.ws.WebSocketSettings; +import org.apache.wicket.protocol.ws.api.IWebSocketConnection; +import org.apache.wicket.protocol.ws.concurrent.Executor; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class HeartBeatWithReconnectTimer extends AbstractHeartBeatTimer { + + private static final Logger LOG = LoggerFactory.getLogger(HeartBeatWithReconnectTimer.class); + + + public HeartBeatWithReconnectTimer(WebSocketSettings webSocketSettings) + { + super(webSocketSettings); + } + + + @Override + protected boolean isTimerEnabled() { + if (webSocketSettings.isUseHeartBeat() == false) + { + if (LOG.isInfoEnabled()) Review comment: no need ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/util/tester/TestWebSocketConnection.java ########## @@ -42,6 +43,41 @@ public TestWebSocketConnection(WebApplication application, String sessionId, IKe this.registryKey = registryKey; } + @Override + public long getLastTimeAlive() { + return 0; + } + + @Override + public boolean isAlive() { + return false; + } + + @Override + public void setAlive(boolean alive) { + + } + + @Override + public void ping() throws IOException { + + } + + @Override + public void pong() throws IOException { + + } + + @Override + public void onPong(ByteBuffer byteBuffer) { + + } + + @Override + public void terminate(String reason) { + close(-1, "abnormally closed"); Review comment: setAlive(false) ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/util/tester/TestWebSocketConnection.java ########## @@ -79,7 +122,7 @@ public IWebSocketConnection sendMessage(byte[] message, int offset, int length) protected abstract void onOutMessage(String message); /** - * A callback method that is called when a text message should be send to the client + * A callback method that is called when a text message should be sent to the client Review comment: I think `d` was correct ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/timer/PingPongHeartBeatTimer.java ########## @@ -0,0 +1,105 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.protocol.ws.timer; + +import java.io.IOException; + +import org.apache.wicket.Application; +import org.apache.wicket.protocol.ws.WebSocketSettings; +import org.apache.wicket.protocol.ws.api.IWebSocketConnection; +import org.apache.wicket.protocol.ws.concurrent.Executor; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class PingPongHeartBeatTimer extends AbstractHeartBeatTimer { + + private static final Logger LOG = LoggerFactory.getLogger(PingPongHeartBeatTimer.class); + + public PingPongHeartBeatTimer(WebSocketSettings webSocketSettings) + { + super(webSocketSettings); + } + + @Override + protected boolean isTimerEnabled() { + if (webSocketSettings.isUsePingPongHeartBeat() == false) + { + if (LOG.isInfoEnabled()) + { + LOG.info("usePingPongHeartBeat is set to false. Thus we won't start heartbeat's sending thread"); + } + return false; + } + return true; + } + + + protected void sendHeartBeats(Application application) + { + final long heartBeatPace = webSocketSettings.getHeartBeatPace(); + final long networkLatencyThreshold = webSocketSettings.getNetworkLatencyThreshold(); + final Executor heartBeatsExecutor = webSocketSettings.getHeartBeatsExecutor(); + final int maxPingRetries = webSocketSettings.getMaxPingRetries(); + for (IWebSocketConnection connection: webSocketSettings.getConnectionRegistry().getConnections(application)) + { + // connection didn't received the PONG from peer terminate it + if (connection.isAlive() == false) + { + if (connection.getLastTimeAlive() - System.currentTimeMillis() > (heartBeatPace + networkLatencyThreshold)) + { + heartBeatsExecutor.run(() -> terminateConnection(connection)); + } + } + else + { + heartBeatsExecutor.run(() -> ping(connection, maxPingRetries)); + } + } + } + + private void ping(IWebSocketConnection connection, final int pingRetryCounter) + { + try + { + connection.ping(); + } + catch (IOException e) + { + if (pingRetryCounter == 0) Review comment: OK. It seems only this timer removes connections. ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/WebSocketSettings.java ########## @@ -139,6 +142,49 @@ public static void set(Application application, WebSocketSettings settings) */ private IWebSocketConnectionFilter connectionFilter; + /** + * Boolean used to determine if ping-pong heart beat will be used. + */ + private boolean useHeartBeat = false; + + + /** + * Boolean used to determine if ping-pong heart beat will be used. + */ + private boolean usePingPongHeartBeat = false; Review comment: What is the difference with `useHeartBeat` ? ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/res/js/wicket-websocket-jquery.js ########## @@ -139,6 +155,22 @@ } }, + heartbeat: function () { + clearTimeout(this.pingTimeout); + // Set a timeout in order to check ping received + this.pingTimeout = setTimeout(() => { + this.ws.close(); Review comment: I didn't expect to see `ws.close()` here. IMO the heartbeat should attempt to send a PING message. If the sending fails or PONG does not come in N millis then try to reconnect M times. ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/timer/HeartBeatWithReconnectTimer.java ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.protocol.ws.timer; + +import java.io.IOException; + +import org.apache.wicket.Application; +import org.apache.wicket.protocol.ws.WebSocketSettings; +import org.apache.wicket.protocol.ws.api.IWebSocketConnection; +import org.apache.wicket.protocol.ws.concurrent.Executor; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class HeartBeatWithReconnectTimer extends AbstractHeartBeatTimer { + + private static final Logger LOG = LoggerFactory.getLogger(HeartBeatWithReconnectTimer.class); + + + public HeartBeatWithReconnectTimer(WebSocketSettings webSocketSettings) + { + super(webSocketSettings); + } + + + @Override + protected boolean isTimerEnabled() { + if (webSocketSettings.isUseHeartBeat() == false) + { + if (LOG.isInfoEnabled()) + { + LOG.info("useHeartBeat is set to false. Thus we won't start heartbeat's sending thread"); + } + return false; + } + return true; + } + + + protected void sendHeartBeats(Application application) + { + final Executor heartBeatsExecutor = webSocketSettings.getHeartBeatsExecutor(); + final int maxPingRetries = webSocketSettings.getMaxPingRetries(); + for (IWebSocketConnection connection: webSocketSettings.getConnectionRegistry().getConnections(application)) + { + heartBeatsExecutor.run(() -> ping(connection, maxPingRetries)); + } + } + + private void ping(IWebSocketConnection connection, final int pingRetryCounter) + { + try + { + // we just sent a binary message + connection.sendMessage(new byte[]{10}); + // if client does not receive message it might try to reconnect + // depending on settings + } + catch (IOException e) + { + ping(connection, pingRetryCounter - 1); Review comment: check for `pingRetryCounter == 0` ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/res/js/wicket-websocket-jquery.js ########## @@ -85,17 +85,32 @@ url += '&context=' + encodeURIComponent(WWS.context); } + // this flag is used at server side to send reconnect event + if (WWS.reconnect) { + url += '&reconnect=' + true + } + url += '&wicket-ajax-baseurl=' + encodeURIComponent(WWS.baseUrl); url += '&wicket-app-name=' + encodeURIComponent(WWS.appName); + console.log(url); Review comment: debug leftover ########## File path: wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/timer/AbstractHeartBeatTimer.java ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.protocol.ws.timer; + +import java.io.IOException; +import java.util.Date; +import java.util.Timer; +import java.util.TimerTask; + +import org.apache.wicket.Application; +import org.apache.wicket.protocol.ws.WebSocketSettings; +import org.apache.wicket.protocol.ws.api.IWebSocketConnection; +import org.apache.wicket.protocol.ws.concurrent.Executor; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public abstract class AbstractHeartBeatTimer { + + private static final Logger LOG = LoggerFactory.getLogger(AbstractHeartBeatTimer.class); + + protected final WebSocketSettings webSocketSettings; + + // internal heartbeat's timer. + private Timer heartBeatsTimer; + + public AbstractHeartBeatTimer(WebSocketSettings webSocketSettings) + { + this.webSocketSettings = webSocketSettings; + } + + public final void start(Application application) + { + if (isTimerEnabled() == false) + { + return; + } + + if (LOG.isInfoEnabled()) + { + LOG.info("Starting thread pushing heart beats"); + } + + TimerTask timerTask = new TimerTask() + { + @Override + public void run() + { + try + { + sendHeartBeats(application); + } + catch (Exception e) + { + LOG.error("Error while checking connections", e); Review comment: What is the benefit of sending PING message from the server to the client if we don't react on "missing client" ? I.e. I'd expect to remove the client connection from the registry if the client cannot be reached in N retries. ########## File path: wicket-native-websocket/wicket-native-websocket-javax/src/main/java/org/apache/wicket/protocol/ws/javax/JavaxWebSocketConnection.java ########## @@ -50,6 +55,69 @@ public JavaxWebSocketConnection(Session session, AbstractWebSocketProcessor webS { super(webSocketProcessor); this.session = Args.notNull(session, "session"); + setAlive(true); + } + + @Override + public long getLastTimeAlive() + { + return lastTimeAlive.get(); + } + + @Override + public boolean isAlive() + { + return alive.get(); + } + + @Override + public void setAlive(boolean alive) + { + if (alive) + { + // is connection if alive we set the timestamp. + this.lastTimeAlive.set(System.currentTimeMillis()); + } + this.alive.set(alive); + } + + @Override + public synchronized void terminate(String reason) + { + close(CloseReason.CloseCodes.CLOSED_ABNORMALLY.getCode(), reason); + } + + @Override + public void ping() throws IOException + { + if (LOG.isDebugEnabled()) + { + LOG.debug("Pinging connection {}", getKey()); + } + ByteBuffer buf = ByteBuffer.wrap(new byte[]{0xA}); + session.getBasicRemote().sendPing(buf); + } + + @Override + public void pong() throws IOException + { + if (LOG.isDebugEnabled()) + { + LOG.debug("Sending unidirectional pon for connection {}", getKey()); Review comment: pon`g` ########## File path: wicket-native-websocket/wicket-native-websocket-javax/src/main/java/org/apache/wicket/protocol/ws/javax/JavaxWebSocketProcessor.java ########## @@ -17,15 +17,19 @@ package org.apache.wicket.protocol.ws.javax; import java.nio.ByteBuffer; +import java.util.List; import javax.websocket.EndpointConfig; import javax.websocket.MessageHandler; +import javax.websocket.PongMessage; Review comment: I still don't understand how JSR 356 spec makes use of javax.websocket.PongMessage -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
