[GitHub] [wicket] martin-g commented on a diff in pull request #509: {WICKET-6969} allow asynchronous pushing of messages.

[GitBox <gi...@apache.org>]

martin-g commented on code in PR #509:
URL: https://github.com/apache/wicket/pull/509#discussion_r847619483


##########
wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/WebSocketSettings.java:
##########
@@ -158,11 +158,24 @@ public void configureSession(IWebSocketSession webSocketSession) {
 	 */
 	private Function<Integer, Boolean> notifyOnCloseEvent = (code) -> true;
 
-	public boolean shouldNotifyOnCloseEvent(int closeCode) {
+	/**
+	 * Flag that allows to use asynchronous push. By default, it is set to false.
+	 */
+	private boolean asynchronousPush = false;
+
+	/**
+	 * The timeout to use for asynchronous push. By default, it is -1 which means use timeout configured by
+	 * sever implementation.

Review Comment:
   ```suggestion
   	 * server implementation.
   ```



##########
wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/WebSocketSettings.java:
##########
@@ -158,11 +158,24 @@ public void configureSession(IWebSocketSession webSocketSession) {
 	 */
 	private Function<Integer, Boolean> notifyOnCloseEvent = (code) -> true;
 
-	public boolean shouldNotifyOnCloseEvent(int closeCode) {
+	/**
+	 * Flag that allows to use asynchronous push. By default, it is set to false.

Review Comment:
   ```suggestion
   	 * Flag that allows to use asynchronous push. By default, it is set to <code>false</code>.
   ```



##########
wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/WebSocketSettings.java:
##########
@@ -305,7 +320,20 @@ public IWebSocketConnectionFilter getConnectionFilter()
 	 */
 	public WebResponse newWebSocketResponse(IWebSocketConnection connection)
 	{
-		return new WebSocketResponse(connection);
+		return new WebSocketResponse(connection, isAsynchronousPush(), getAsynchronousPushTimeout());

Review Comment:
   ```suggestion
   		return newWebSocketResponse(connection, isAsynchronousPush(), getAsynchronousPushTimeout());
   ```



##########
wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/WebSocketSettings.java:
##########
@@ -305,7 +320,20 @@ public IWebSocketConnectionFilter getConnectionFilter()
 	 */
 	public WebResponse newWebSocketResponse(IWebSocketConnection connection)
 	{
-		return new WebSocketResponse(connection);
+		return new WebSocketResponse(connection, isAsynchronousPush(), getAsynchronousPushTimeout());
+	}
+
+	/**
+	 * A factory method for the {@link org.apache.wicket.request.http.WebResponse}
+	 * that should be used to write the response back to the client/browser
+	 *
+	 * @param connection
+	 *              The active web socket connection

Review Comment:
   missing `@param`s for  `asynchronousPush` and `timeout`



##########
wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/WebSocketRequestHandler.java:
##########
@@ -97,6 +119,27 @@ public void push(byte[] message, int offset, int length)
 		}
 	}
 
+	@Override
+	public Future<Void> pushAsync(byte[] message, int offset, int length)
+	{
+		return pushAsync(message, offset, length, -1);
+	}
+
+	@Override
+	public Future<Void> pushAsync(byte[] message, int offset, int length, long timeout)
+	{
+		if (connection.isOpen())
+		{
+			Args.notNull(message, "message");
+			return connection.sendMessageAsync(message, offset, length, timeout);
+		}
+		else
+		{
+			LOG.warn("The websocket connection is already closed. Cannot push the binary message '{}'", message);
+		}
+		return null;

Review Comment:
   ```suggestion
   		return java.util.concurrent.CompletableFuture.completedFuture(null);
   ```



##########
wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/WebSocketResponse.java:
##########
@@ -87,13 +100,34 @@ public void close()
 			{
 				if (text != null)
 				{
-					connection.sendMessage(text.toString());
+					if (asynchronous)
+					{
+						if (timeout > 0)
+						{
+							connection.sendMessageAsync(text.toString(), timeout);
+						}
+						else
+						{
+							connection.sendMessageAsync(text.toString());
+						}
+					}
+					else
+					{
+						connection.sendMessage(text.toString());
+					}
 					text = null;
 				}
 				else if (binary != null)
 				{
 					byte[] bytes = binary.toByteArray();
-					connection.sendMessage(bytes, 0, bytes.length);
+					if (asynchronous)
+					{
+						connection.sendMessageAsync(bytes, 0, bytes.length);

Review Comment:
   Why this method does not use the `timeout` ?



##########
wicket-native-websocket/wicket-native-websocket-core/src/main/java/org/apache/wicket/protocol/ws/api/WebSocketRequestHandler.java:
##########
@@ -77,6 +78,27 @@ public void push(CharSequence message)
 		}
 	}
 
+	@Override
+	public Future<Void> pushAsync(CharSequence message, long timeout)
+	{
+		if (connection.isOpen())
+		{
+			Args.notNull(message, "message");
+			return connection.sendMessageAsync(message.toString(), timeout);
+		}
+		else
+		{
+			LOG.warn("The websocket connection is already closed. Cannot push the text message '{}'", message);
+		}
+		return null;

Review Comment:
   Better return `java.util.concurrent.CompletableFuture#completedFuture(null)`



-- 
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: commits-unsubscribe@wicket.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org