From a2feec7511f312a2196cf275263699ae948ebd96 Mon Sep 17 00:00:00 2001 From: Mikael Grankvist Date: Thu, 19 Dec 2024 09:28:17 +0200 Subject: [PATCH 1/6] feat: queue message payloads Add sent payloads to message queue and resend if no response to message inside MaxMessageSuspendTimeout fixes #20507 --- .../client/communication/MessageHandler.java | 4 +- .../client/communication/MessageSender.java | 77 ++++++++++++++++++- 2 files changed, 77 insertions(+), 4 deletions(-) diff --git a/flow-client/src/main/java/com/vaadin/client/communication/MessageHandler.java b/flow-client/src/main/java/com/vaadin/client/communication/MessageHandler.java index 98533e45c1e..9963c03cbf0 100644 --- a/flow-client/src/main/java/com/vaadin/client/communication/MessageHandler.java +++ b/flow-client/src/main/java/com/vaadin/client/communication/MessageHandler.java @@ -295,10 +295,10 @@ protected void handleJSON(final ValueMap valueMap) { } /** - * Should only prepare resync after the if (locked || + * Should only prepare resync after the (locked || * !isNextExpectedMessage(serverId)) {...} since * stateTree.repareForResync() will remove the nodes, and if locked is - * true, it will return without handling the message, thus won't adding + * true, it will return without handling the message, thus won't add * nodes back. * * This is related to https://github.com/vaadin/flow/issues/8699 It diff --git a/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java b/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java index 6182fde8123..61ccc0bff96 100644 --- a/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java +++ b/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java @@ -15,7 +15,11 @@ */ package com.vaadin.client.communication; +import java.util.ArrayList; +import java.util.List; + import com.google.gwt.core.client.GWT; +import com.google.gwt.user.client.Timer; import com.vaadin.client.ConnectionIndicator; import com.vaadin.client.Console; @@ -67,6 +71,10 @@ public enum ResynchronizationState { private JsonObject pushPendingMessage; + private List messageQueue = new ArrayList<>(); + + private Timer resendMessageTimer; + /** * Creates a new instance connected to the given registry. * @@ -146,6 +154,8 @@ private void doSendInvocationsToServer() { if (resynchronizationState == ResynchronizationState.SEND_TO_SERVER) { resynchronizationState = ResynchronizationState.WAITING_FOR_RESPONSE; Console.warn("Resynchronizing from server"); + messageQueue.clear(); + resetTimer(); extraJson.put(ApplicationConstants.RESYNCHRONIZE_ID, true); } if (showLoadingIndicator) { @@ -166,7 +176,6 @@ protected void send(final JsonArray reqInvocations, final JsonObject extraJson) { registry.getRequestResponseTracker().startRequest(); send(preparePayload(reqInvocations, extraJson)); - } private JsonObject preparePayload(final JsonArray reqInvocations, @@ -192,12 +201,33 @@ private JsonObject preparePayload(final JsonArray reqInvocations, /** * Sends an asynchronous or synchronous UIDL request to the server using the - * given URI. + * given URI. Adds message to message queue and postpones sending if queue + * not empty. * * @param payload * The contents of the request to send */ public void send(final JsonObject payload) { + if (!registry.getRequestResponseTracker().hasActiveRequest()) { + // Someone called send directly as request not set active. + // If queue empty add message and wait to send. + if (!messageQueue.isEmpty()) { + messageQueue.add(payload); + return; + } + } + messageQueue.add(payload); + sendPayload(payload); + } + + /** + * Sends an asynchronous or synchronous UIDL request to the server using the + * given URI. + * + * @param payload + * The contents of the request to send + */ + private void sendPayload(final JsonObject payload) { if (push != null && push.isBidirectional()) { // When using bidirectional transport, the payload is not resent // to the server during reconnection attempts. @@ -211,6 +241,30 @@ public void send(final JsonObject payload) { } else { Console.debug("send XHR"); registry.getXhrConnection().send(payload); + + resetTimer(); + // resend last payload if response hasn't come in. + resendMessageTimer = new Timer() { + @Override + public void run() { + resendMessageTimer = null; + if (!registry.getRequestResponseTracker() + .hasActiveRequest()) { + registry.getRequestResponseTracker().startRequest(); + } + registry.getXhrConnection().send(payload); + } + }; + resendMessageTimer.schedule(registry.getApplicationConfiguration() + .getMaxMessageSuspendTimeout() + 500); + } + } + + private void resetTimer() { + if (resendMessageTimer != null) { + resendMessageTimer.cancel(); + resendMessageTimer = null; + } } @@ -289,6 +343,8 @@ public String getCommunicationMethodName() { */ public void resynchronize() { if (requestResynchronize()) { + messageQueue.clear(); + resetTimer(); sendInvocationsToServer(); } } @@ -311,12 +367,29 @@ public void setClientToServerMessageId(int nextExpectedId, boolean force) { ApplicationConstants.CLIENT_TO_SERVER_ID) < nextExpectedId) { pushPendingMessage = null; } + if (!messageQueue.isEmpty()) { + synchronized (messageQueue) { + // If queued message is the expected one. remove from queue + // and sen next message if any. + if (messageQueue.get(0) + .getNumber(ApplicationConstants.CLIENT_TO_SERVER_ID) + + 1 == nextExpectedId) { + resetTimer(); + messageQueue.remove(0); + if (!messageQueue.isEmpty()) { + sendPayload(messageQueue.get(0)); + } + } + } + } return; } if (force) { Console.debug( "Forced update of clientId to " + clientToServerMessageId); clientToServerMessageId = nextExpectedId; + messageQueue.clear(); + resetTimer(); return; } From ca6d8e7ca087c4da71ae27187bfb5592032f776c Mon Sep 17 00:00:00 2001 From: Mikael Grankvist Date: Fri, 3 Jan 2025 13:58:06 +0200 Subject: [PATCH 2/6] Add test for re-request Fix queued message send timing. --- .../client/communication/MessageSender.java | 39 ++++--- .../communication/RequestResponseTracker.java | 5 +- .../communication/ServerRpcHandler.java | 7 +- flow-tests/pom.xml | 1 + flow-tests/test-client-queue/pom.xml | 63 +++++++++++ .../vaadin/flow/misc/ui/CustomService.java | 107 ++++++++++++++++++ .../vaadin/flow/misc/ui/CustomServlet.java | 37 ++++++ .../misc/ui/CustomUidlRequestHandler.java | 59 ++++++++++ .../flow/misc/ui/TestNoResponseView.java | 45 ++++++++ .../com/vaadin/flow/misc/ui/NoResponseIT.java | 106 +++++++++++++++++ 10 files changed, 448 insertions(+), 21 deletions(-) create mode 100644 flow-tests/test-client-queue/pom.xml create mode 100644 flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomService.java create mode 100644 flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomServlet.java create mode 100644 flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomUidlRequestHandler.java create mode 100644 flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/TestNoResponseView.java create mode 100644 flow-tests/test-client-queue/src/test/java/com/vaadin/flow/misc/ui/NoResponseIT.java diff --git a/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java b/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java index 61ccc0bff96..f9d951324ee 100644 --- a/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java +++ b/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java @@ -127,7 +127,13 @@ private void doSendInvocationsToServer() { JsonObject payload = pushPendingMessage; pushPendingMessage = null; registry.getRequestResponseTracker().startRequest(); - send(payload); + sendPayload(payload); + return; + } else if (hasQueuedMessages() && resendMessageTimer == null) { + if (!registry.getRequestResponseTracker().hasActiveRequest()) { + registry.getRequestResponseTracker().startRequest(); + } + sendPayload(messageQueue.get(0)); return; } @@ -186,10 +192,6 @@ private JsonObject preparePayload(final JsonArray reqInvocations, payload.put(ApplicationConstants.CSRF_TOKEN, csrfToken); } payload.put(ApplicationConstants.RPC_INVOCATIONS, reqInvocations); - payload.put(ApplicationConstants.SERVER_SYNC_ID, - registry.getMessageHandler().getLastSeenServerSyncId()); - payload.put(ApplicationConstants.CLIENT_TO_SERVER_ID, - clientToServerMessageId++); if (extraJson != null) { for (String key : extraJson.keys()) { JsonValue value = extraJson.get(key); @@ -208,13 +210,9 @@ private JsonObject preparePayload(final JsonArray reqInvocations, * The contents of the request to send */ public void send(final JsonObject payload) { - if (!registry.getRequestResponseTracker().hasActiveRequest()) { - // Someone called send directly as request not set active. - // If queue empty add message and wait to send. - if (!messageQueue.isEmpty()) { - messageQueue.add(payload); - return; - } + if (!messageQueue.isEmpty()) { + messageQueue.add(payload); + return; } messageQueue.add(payload); sendPayload(payload); @@ -228,6 +226,11 @@ public void send(final JsonObject payload) { * The contents of the request to send */ private void sendPayload(final JsonObject payload) { + payload.put(ApplicationConstants.SERVER_SYNC_ID, + registry.getMessageHandler().getLastSeenServerSyncId()); + payload.put(ApplicationConstants.CLIENT_TO_SERVER_ID, + clientToServerMessageId++); + if (push != null && push.isBidirectional()) { // When using bidirectional transport, the payload is not resent // to the server during reconnection attempts. @@ -247,7 +250,9 @@ private void sendPayload(final JsonObject payload) { resendMessageTimer = new Timer() { @Override public void run() { - resendMessageTimer = null; + resendMessageTimer + .schedule(registry.getApplicationConfiguration() + .getMaxMessageSuspendTimeout() + 500); if (!registry.getRequestResponseTracker() .hasActiveRequest()) { registry.getRequestResponseTracker().startRequest(); @@ -264,7 +269,6 @@ private void resetTimer() { if (resendMessageTimer != null) { resendMessageTimer.cancel(); resendMessageTimer = null; - } } @@ -376,9 +380,6 @@ public void setClientToServerMessageId(int nextExpectedId, boolean force) { + 1 == nextExpectedId) { resetTimer(); messageQueue.remove(0); - if (!messageQueue.isEmpty()) { - sendPayload(messageQueue.get(0)); - } } } } @@ -445,4 +446,8 @@ void clearResynchronizationState() { ResynchronizationState getResynchronizationState() { return resynchronizationState; } + + public boolean hasQueuedMessages() { + return !messageQueue.isEmpty(); + } } diff --git a/flow-client/src/main/java/com/vaadin/client/communication/RequestResponseTracker.java b/flow-client/src/main/java/com/vaadin/client/communication/RequestResponseTracker.java index 0d08be7644a..e41df551573 100644 --- a/flow-client/src/main/java/com/vaadin/client/communication/RequestResponseTracker.java +++ b/flow-client/src/main/java/com/vaadin/client/communication/RequestResponseTracker.java @@ -110,9 +110,10 @@ public void endRequest() { hasActiveRequest = false; if ((registry.getUILifecycle().isRunning() - && registry.getServerRpcQueue().isFlushPending()) + && (registry.getServerRpcQueue().isFlushPending()) || registry.getMessageSender() - .getResynchronizationState() == ResynchronizationState.SEND_TO_SERVER) { + .getResynchronizationState() == ResynchronizationState.SEND_TO_SERVER + || registry.getMessageSender().hasQueuedMessages())) { // Send the pending RPCs immediately. // This might be an unnecessary optimization as ServerRpcQueue has a // finally scheduled command which trigger the send if we do not do diff --git a/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java b/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java index 802329461af..a9a8a7166ad 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java @@ -113,7 +113,7 @@ public RpcRequest(String jsonString, boolean isSyncIdCheckEnabled) { this.csrfToken = csrfToken; } - if (isSyncIdCheckEnabled) { + if (isSyncIdCheckEnabled && !isUnloadBeaconRequest()) { syncId = (int) json .getNumber(ApplicationConstants.SERVER_SYNC_ID); } else { @@ -131,7 +131,10 @@ public RpcRequest(String jsonString, boolean isSyncIdCheckEnabled) { clientToServerMessageId = (int) json .getNumber(ApplicationConstants.CLIENT_TO_SERVER_ID); } else { - getLogger().warn("Server message without client id received"); + if(!isUnloadBeaconRequest()) { + getLogger().warn( + "Server message without client id received"); + } clientToServerMessageId = -1; } invocations = json.getArray(ApplicationConstants.RPC_INVOCATIONS); diff --git a/flow-tests/pom.xml b/flow-tests/pom.xml index aa7c2e83a08..d7946ba9863 100644 --- a/flow-tests/pom.xml +++ b/flow-tests/pom.xml @@ -335,6 +335,7 @@ test-react-adapter test-react-adapter/pom-production.xml test-legacy-frontend + test-client-queue diff --git a/flow-tests/test-client-queue/pom.xml b/flow-tests/test-client-queue/pom.xml new file mode 100644 index 00000000000..9e0908c433c --- /dev/null +++ b/flow-tests/test-client-queue/pom.xml @@ -0,0 +1,63 @@ + + + 4.0.0 + + flow-tests + com.vaadin + 24.7-SNAPSHOT + + flow-client-queue-test + Test Flow client queue + + war + + true + + true + + + + + com.vaadin + flow-test-resources + ${project.version} + + + com.vaadin + vaadin-dev-server + ${project.version} + + + com.vaadin + flow-html-components-testbench + ${project.version} + test + + + + + + + + com.vaadin + flow-maven-plugin + + + + prepare-frontend + + + + + + + + + + org.eclipse.jetty.ee10 + jetty-ee10-maven-plugin + + + + + diff --git a/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomService.java b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomService.java new file mode 100644 index 00000000000..b8abbb67e6b --- /dev/null +++ b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomService.java @@ -0,0 +1,107 @@ +/* + * Copyright 2000-2025 Vaadin Ltd. + * + * Licensed 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 com.vaadin.flow.misc.ui; + +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; + +import com.vaadin.flow.function.DeploymentConfiguration; +import com.vaadin.flow.i18n.TranslationFileRequestHandler; +import com.vaadin.flow.internal.DevModeHandler; +import com.vaadin.flow.internal.DevModeHandlerManager; +import com.vaadin.flow.server.Mode; +import com.vaadin.flow.server.RequestHandler; +import com.vaadin.flow.server.ServiceException; +import com.vaadin.flow.server.UnsupportedBrowserHandler; +import com.vaadin.flow.server.VaadinServlet; +import com.vaadin.flow.server.VaadinServletService; +import com.vaadin.flow.server.communication.FaviconHandler; +import com.vaadin.flow.server.communication.HeartbeatHandler; +import com.vaadin.flow.server.communication.IndexHtmlRequestHandler; +import com.vaadin.flow.server.communication.JavaScriptBootstrapHandler; +import com.vaadin.flow.server.communication.PushRequestHandler; +import com.vaadin.flow.server.communication.PwaHandler; +import com.vaadin.flow.server.communication.SessionRequestHandler; +import com.vaadin.flow.server.communication.StreamRequestHandler; +import com.vaadin.flow.server.communication.WebComponentBootstrapHandler; +import com.vaadin.flow.server.communication.WebComponentProvider; + +public class CustomService extends VaadinServletService { + + public CustomService(VaadinServlet servlet, + DeploymentConfiguration deploymentConfiguration) { + super(servlet, deploymentConfiguration); + } + + @Override + protected List createRequestHandlers() + throws ServiceException { + List handlers = new ArrayList<>(); + handlers.add(new FaviconHandler()); + handlers.add(new JavaScriptBootstrapHandler()); + handlers.add(new SessionRequestHandler()); + handlers.add(new HeartbeatHandler()); + + handlers.add(new CustomUidlRequestHandler()); + + handlers.add(new UnsupportedBrowserHandler()); + + handlers.add(new StreamRequestHandler()); + + handlers.add(new PwaHandler(() -> getPwaRegistry())); + + handlers.add(new TranslationFileRequestHandler( + getInstantiator().getI18NProvider())); + + handlers.add(new WebComponentBootstrapHandler()); + handlers.add(new WebComponentProvider()); + + Mode mode = getDeploymentConfiguration().getMode(); + if (mode == Mode.DEVELOPMENT_FRONTEND_LIVERELOAD + || mode == Mode.DEVELOPMENT_BUNDLE) { + Optional handlerManager = DevModeHandlerManager + .getDevModeHandler(this); + if (handlerManager.isPresent()) { + DevModeHandler devModeHandler = handlerManager.get(); + // WebComponentProvider handler should run before DevModeHandler + // to avoid responding with html contents when dev bundle is + // not ready (e.g. dev-mode-not-ready.html) + handlers.stream().filter(WebComponentProvider.class::isInstance) + .findFirst().map(handlers::indexOf) + .ifPresentOrElse(idx -> { + handlers.add(idx, devModeHandler); + }, () -> handlers.add(devModeHandler)); + } + } + + // PushRequestHandler should run before DevModeHandler to avoid + // responding with html contents when dev mode server is not ready + // (e.g. dev-mode-not-ready.html) + if (isAtmosphereAvailable()) { + try { + handlers.add(new PushRequestHandler(this)); + } catch (ServiceException e) { + // Atmosphere init failed. Push won't work but we don't throw a + // service exception as we don't want to prevent non-push + // applications from working + } + } + + handlers.add(0, new IndexHtmlRequestHandler()); + return handlers; + } +} diff --git a/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomServlet.java b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomServlet.java new file mode 100644 index 00000000000..7aba1806e05 --- /dev/null +++ b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomServlet.java @@ -0,0 +1,37 @@ +/* + * Copyright 2000-2024 Vaadin Ltd. + * + * Licensed 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 com.vaadin.flow.misc.ui; + +import jakarta.servlet.annotation.WebServlet; + +import com.vaadin.flow.function.DeploymentConfiguration; +import com.vaadin.flow.server.ServiceException; +import com.vaadin.flow.server.VaadinServlet; +import com.vaadin.flow.server.VaadinServletService; + +@WebServlet(urlPatterns = "/*", asyncSupported = true) +public class CustomServlet extends VaadinServlet { + + @Override + protected VaadinServletService createServletService( + DeploymentConfiguration deploymentConfiguration) + throws ServiceException { + CustomService service = new CustomService(this, + deploymentConfiguration); + service.init(); + return service; + } +} diff --git a/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomUidlRequestHandler.java b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomUidlRequestHandler.java new file mode 100644 index 00000000000..b65460108fb --- /dev/null +++ b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomUidlRequestHandler.java @@ -0,0 +1,59 @@ +/* + * Copyright 2000-2025 Vaadin Ltd. + * + * Licensed 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 com.vaadin.flow.misc.ui; + +import java.io.IOException; +import java.util.Optional; + +import com.vaadin.flow.server.VaadinRequest; +import com.vaadin.flow.server.VaadinResponse; +import com.vaadin.flow.server.VaadinSession; +import com.vaadin.flow.server.communication.UidlRequestHandler; + +public class CustomUidlRequestHandler extends UidlRequestHandler { + + public static boolean emptyResponse = false; + + @Override + public boolean synchronizedHandleRequest(VaadinSession session, + VaadinRequest request, VaadinResponse response) throws IOException { + if (emptyResponse) { + emptyResponse = false; + commitEmptyResponse(response); + return true; + } + return super.synchronizedHandleRequest(session, request, response); + } + + @Override + public Optional synchronizedHandleRequest( + VaadinSession session, VaadinRequest request, + VaadinResponse response, String requestBody) + throws IOException, UnsupportedOperationException { + + if (emptyResponse) { + emptyResponse = false; + return Optional.of(() -> commitEmptyResponse(response)); + } + return super.synchronizedHandleRequest(session, request, response, + requestBody); + } + + private void commitEmptyResponse(VaadinResponse response) + throws IOException { + commitJsonResponse(response, "for(;;);[{}]"); + } +} diff --git a/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/TestNoResponseView.java b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/TestNoResponseView.java new file mode 100644 index 00000000000..88eac28f1f4 --- /dev/null +++ b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/TestNoResponseView.java @@ -0,0 +1,45 @@ +/* + * Copyright 2000-2025 Vaadin Ltd. + * + * Licensed 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 com.vaadin.flow.misc.ui; + +import com.vaadin.flow.component.html.Div; +import com.vaadin.flow.component.html.NativeButton; +import com.vaadin.flow.router.Route; + +@Route("no-response") +public class TestNoResponseView extends Div { + + public static final String DELAY_NEXT_RESPONSE = "delay-next"; + public static final String ADD = "add"; + public static final String ADDED_PREDICATE = "added_"; + + private int elements = 0; + + public TestNoResponseView() { + NativeButton delayNext = new NativeButton("\"Delay\" next response", + event -> CustomUidlRequestHandler.emptyResponse = true); + delayNext.setId(DELAY_NEXT_RESPONSE); + + NativeButton addElement = new NativeButton("Add element", event -> { + Div addedElement = new Div("Added element"); + addedElement.setId(ADDED_PREDICATE + elements++); + add(addedElement); + }); + addElement.setId(ADD); + + add(delayNext, addElement); + } +} diff --git a/flow-tests/test-client-queue/src/test/java/com/vaadin/flow/misc/ui/NoResponseIT.java b/flow-tests/test-client-queue/src/test/java/com/vaadin/flow/misc/ui/NoResponseIT.java new file mode 100644 index 00000000000..30eb3a621cf --- /dev/null +++ b/flow-tests/test-client-queue/src/test/java/com/vaadin/flow/misc/ui/NoResponseIT.java @@ -0,0 +1,106 @@ +package com.vaadin.flow.misc.ui; + +import java.util.logging.Level; + +import org.junit.Assert; +import org.junit.Test; +import org.openqa.selenium.TimeoutException; + +import com.vaadin.flow.component.html.testbench.DivElement; +import com.vaadin.flow.component.html.testbench.NativeButtonElement; +import com.vaadin.flow.testutil.ChromeBrowserTest; + +import static com.vaadin.flow.misc.ui.TestNoResponseView.ADD; +import static com.vaadin.flow.misc.ui.TestNoResponseView.ADDED_PREDICATE; +import static com.vaadin.flow.misc.ui.TestNoResponseView.DELAY_NEXT_RESPONSE; + +public class NoResponseIT extends ChromeBrowserTest { + + @Override + protected String getTestPath() { + return "/no-response"; + } + + @Test + public void noResponseForRequest_clientResendsRequest_serverAnswersCorrectly() { + open(); + + try { + waitUntil(driver -> $(NativeButtonElement.class) + .withId(DELAY_NEXT_RESPONSE).exists()); + } catch (TimeoutException te) { + Assert.fail("Expected 'delay next' button wasn't found"); + } + + // Add element normally + $(NativeButtonElement.class).id(ADD).click(); + Assert.assertTrue( + $(DivElement.class).id(ADDED_PREDICATE + 0).isDisplayed()); + + // Request null response for next add + $(NativeButtonElement.class).id(DELAY_NEXT_RESPONSE).click(); + + $(NativeButtonElement.class).id(ADD).click(); + + Assert.assertEquals("No expected empty response found", 1, + getLogEntries(Level.WARNING).stream() + .filter(logEntry -> logEntry.getMessage().contains( + "Response didn't contain a server id.")) + .count()); + + try { + waitUntil(driver -> $(DivElement.class).withId(ADDED_PREDICATE + 1) + .exists()); + } catch (TimeoutException te) { + Assert.fail( + "New element was not added though client should re-send request."); + } + + } + + @Test + public void clickWhileRequestPending_clientQueuesRequests_messagesSentCorrectly() { + open(); + + try { + waitUntil(driver -> $(NativeButtonElement.class) + .withId(DELAY_NEXT_RESPONSE).exists()); + } catch (TimeoutException te) { + Assert.fail("Expected 'delay next' button wasn't found"); + } + + // Add element normally + $(NativeButtonElement.class).id(ADD).click(); + Assert.assertTrue( + $(DivElement.class).id(ADDED_PREDICATE + 0).isDisplayed()); + + // Request null response for next add + $(NativeButtonElement.class).id(DELAY_NEXT_RESPONSE).click(); + + $(NativeButtonElement.class).id(ADD).click(); + $(NativeButtonElement.class).id(ADD).click(); + + Assert.assertEquals("No expected empty response found", 1, + getLogEntries(Level.WARNING).stream() + .filter(logEntry -> logEntry.getMessage().contains( + "Response didn't contain a server id.")) + .count()); + + try { + waitUntil(driver -> $(DivElement.class).withId(ADDED_PREDICATE + 1) + .exists()); + } catch (TimeoutException te) { + Assert.fail( + "New element was not added though client should re-send request."); + } + + try { + waitUntil(driver -> $(DivElement.class).withId(ADDED_PREDICATE + 2) + .exists()); + } catch (TimeoutException te) { + Assert.fail( + "Second new element was not added though client should queue request."); + } + + } +} From b8ef0d0317b0813143659571fad7e4a3c98f3e56 Mon Sep 17 00:00:00 2001 From: Mikael Grankvist Date: Wed, 8 Jan 2025 13:17:59 +0200 Subject: [PATCH 3/6] fix test server response format server --- .../java/com/vaadin/client/communication/MessageSender.java | 6 ++++++ flow-client/src/test/frontend/FlowTests.ts | 2 +- .../vaadin/flow/server/communication/ServerRpcHandler.java | 6 +++--- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java b/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java index f9d951324ee..8dc9eb0463c 100644 --- a/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java +++ b/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java @@ -231,6 +231,12 @@ private void sendPayload(final JsonObject payload) { payload.put(ApplicationConstants.CLIENT_TO_SERVER_ID, clientToServerMessageId++); + if (!registry.getRequestResponseTracker().hasActiveRequest()) { + // Direct calls to send from outside probably have not started + // request. + registry.getRequestResponseTracker().startRequest(); + } + if (push != null && push.isBidirectional()) { // When using bidirectional transport, the payload is not resent // to the server during reconnection attempts. diff --git a/flow-client/src/test/frontend/FlowTests.ts b/flow-client/src/test/frontend/FlowTests.ts index 778c4137131..24b4edcf0ec 100644 --- a/flow-client/src/test/frontend/FlowTests.ts +++ b/flow-client/src/test/frontend/FlowTests.ts @@ -748,7 +748,7 @@ function stubServerRemoteFunction( handlers.leaveNavigation(); } } - req.respond(200, { 'content-type': 'application/json' }, 'for(;;);[{}]'); + req.respond(200, {'content-type': 'application/json'}, 'for(;;);[{"syncId":' + (payload["syncId"] + 1) + ',"clientId":' + (payload["clientId"] + 1) + '}]'); }); } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java b/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java index a9a8a7166ad..1b07e9466d7 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java @@ -131,9 +131,9 @@ public RpcRequest(String jsonString, boolean isSyncIdCheckEnabled) { clientToServerMessageId = (int) json .getNumber(ApplicationConstants.CLIENT_TO_SERVER_ID); } else { - if(!isUnloadBeaconRequest()) { - getLogger().warn( - "Server message without client id received"); + if (!isUnloadBeaconRequest()) { + getLogger() + .warn("Server message without client id received"); } clientToServerMessageId = -1; } From f50afba01185fe61dfcea7479f92ee7b54e84df2 Mon Sep 17 00:00:00 2001 From: Mikael Grankvist Date: Fri, 10 Jan 2025 09:24:15 +0200 Subject: [PATCH 4/6] Make custom service simpler Fix concurrent issue with custom uidl handler. --- .../vaadin/flow/misc/ui/CustomService.java | 77 ++----------------- .../misc/ui/CustomUidlRequestHandler.java | 12 +-- .../flow/misc/ui/TestNoResponseView.java | 4 +- 3 files changed, 18 insertions(+), 75 deletions(-) diff --git a/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomService.java b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomService.java index b8abbb67e6b..cdf340709c5 100644 --- a/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomService.java +++ b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomService.java @@ -15,30 +15,14 @@ */ package com.vaadin.flow.misc.ui; -import java.util.ArrayList; import java.util.List; -import java.util.Optional; import com.vaadin.flow.function.DeploymentConfiguration; -import com.vaadin.flow.i18n.TranslationFileRequestHandler; -import com.vaadin.flow.internal.DevModeHandler; -import com.vaadin.flow.internal.DevModeHandlerManager; -import com.vaadin.flow.server.Mode; import com.vaadin.flow.server.RequestHandler; import com.vaadin.flow.server.ServiceException; -import com.vaadin.flow.server.UnsupportedBrowserHandler; import com.vaadin.flow.server.VaadinServlet; import com.vaadin.flow.server.VaadinServletService; -import com.vaadin.flow.server.communication.FaviconHandler; -import com.vaadin.flow.server.communication.HeartbeatHandler; -import com.vaadin.flow.server.communication.IndexHtmlRequestHandler; -import com.vaadin.flow.server.communication.JavaScriptBootstrapHandler; -import com.vaadin.flow.server.communication.PushRequestHandler; -import com.vaadin.flow.server.communication.PwaHandler; -import com.vaadin.flow.server.communication.SessionRequestHandler; -import com.vaadin.flow.server.communication.StreamRequestHandler; -import com.vaadin.flow.server.communication.WebComponentBootstrapHandler; -import com.vaadin.flow.server.communication.WebComponentProvider; +import com.vaadin.flow.server.communication.UidlRequestHandler; public class CustomService extends VaadinServletService { @@ -50,58 +34,13 @@ public CustomService(VaadinServlet servlet, @Override protected List createRequestHandlers() throws ServiceException { - List handlers = new ArrayList<>(); - handlers.add(new FaviconHandler()); - handlers.add(new JavaScriptBootstrapHandler()); - handlers.add(new SessionRequestHandler()); - handlers.add(new HeartbeatHandler()); - - handlers.add(new CustomUidlRequestHandler()); - - handlers.add(new UnsupportedBrowserHandler()); - - handlers.add(new StreamRequestHandler()); - - handlers.add(new PwaHandler(() -> getPwaRegistry())); - - handlers.add(new TranslationFileRequestHandler( - getInstantiator().getI18NProvider())); - - handlers.add(new WebComponentBootstrapHandler()); - handlers.add(new WebComponentProvider()); - - Mode mode = getDeploymentConfiguration().getMode(); - if (mode == Mode.DEVELOPMENT_FRONTEND_LIVERELOAD - || mode == Mode.DEVELOPMENT_BUNDLE) { - Optional handlerManager = DevModeHandlerManager - .getDevModeHandler(this); - if (handlerManager.isPresent()) { - DevModeHandler devModeHandler = handlerManager.get(); - // WebComponentProvider handler should run before DevModeHandler - // to avoid responding with html contents when dev bundle is - // not ready (e.g. dev-mode-not-ready.html) - handlers.stream().filter(WebComponentProvider.class::isInstance) - .findFirst().map(handlers::indexOf) - .ifPresentOrElse(idx -> { - handlers.add(idx, devModeHandler); - }, () -> handlers.add(devModeHandler)); + List requestHandlers = super.createRequestHandlers(); + requestHandlers.replaceAll(handler -> { + if (handler instanceof UidlRequestHandler) { + return new CustomUidlRequestHandler(); } - } - - // PushRequestHandler should run before DevModeHandler to avoid - // responding with html contents when dev mode server is not ready - // (e.g. dev-mode-not-ready.html) - if (isAtmosphereAvailable()) { - try { - handlers.add(new PushRequestHandler(this)); - } catch (ServiceException e) { - // Atmosphere init failed. Push won't work but we don't throw a - // service exception as we don't want to prevent non-push - // applications from working - } - } - - handlers.add(0, new IndexHtmlRequestHandler()); - return handlers; + return handler; + }); + return requestHandlers; } } diff --git a/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomUidlRequestHandler.java b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomUidlRequestHandler.java index b65460108fb..976e04555d5 100644 --- a/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomUidlRequestHandler.java +++ b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/CustomUidlRequestHandler.java @@ -16,7 +16,9 @@ package com.vaadin.flow.misc.ui; import java.io.IOException; +import java.util.HashSet; import java.util.Optional; +import java.util.Set; import com.vaadin.flow.server.VaadinRequest; import com.vaadin.flow.server.VaadinResponse; @@ -25,13 +27,13 @@ public class CustomUidlRequestHandler extends UidlRequestHandler { - public static boolean emptyResponse = false; + public static Set emptyResponse = new HashSet(); @Override public boolean synchronizedHandleRequest(VaadinSession session, VaadinRequest request, VaadinResponse response) throws IOException { - if (emptyResponse) { - emptyResponse = false; + if (emptyResponse.contains(session)) { + emptyResponse.remove(session); commitEmptyResponse(response); return true; } @@ -44,8 +46,8 @@ public Optional synchronizedHandleRequest( VaadinResponse response, String requestBody) throws IOException, UnsupportedOperationException { - if (emptyResponse) { - emptyResponse = false; + if (emptyResponse.contains(session)) { + emptyResponse.remove(session); return Optional.of(() -> commitEmptyResponse(response)); } return super.synchronizedHandleRequest(session, request, response, diff --git a/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/TestNoResponseView.java b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/TestNoResponseView.java index 88eac28f1f4..f6c97bc8373 100644 --- a/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/TestNoResponseView.java +++ b/flow-tests/test-client-queue/src/main/java/com/vaadin/flow/misc/ui/TestNoResponseView.java @@ -18,6 +18,7 @@ import com.vaadin.flow.component.html.Div; import com.vaadin.flow.component.html.NativeButton; import com.vaadin.flow.router.Route; +import com.vaadin.flow.server.VaadinSession; @Route("no-response") public class TestNoResponseView extends Div { @@ -30,7 +31,8 @@ public class TestNoResponseView extends Div { public TestNoResponseView() { NativeButton delayNext = new NativeButton("\"Delay\" next response", - event -> CustomUidlRequestHandler.emptyResponse = true); + event -> CustomUidlRequestHandler.emptyResponse + .add(VaadinSession.getCurrent())); delayNext.setId(DELAY_NEXT_RESPONSE); NativeButton addElement = new NativeButton("Add element", event -> { From d28282bf08b21160eb1219efdbdae6909b72d929 Mon Sep 17 00:00:00 2001 From: Mikael Grankvist Date: Fri, 10 Jan 2025 12:33:09 +0200 Subject: [PATCH 5/6] use the has method clear queue for push messaging. --- .../java/com/vaadin/client/communication/MessageSender.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java b/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java index 8dc9eb0463c..6e2e3921643 100644 --- a/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java +++ b/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java @@ -210,7 +210,7 @@ private JsonObject preparePayload(final JsonArray reqInvocations, * The contents of the request to send */ public void send(final JsonObject payload) { - if (!messageQueue.isEmpty()) { + if (hasQueuedMessages()) { messageQueue.add(payload); return; } @@ -238,6 +238,7 @@ private void sendPayload(final JsonObject payload) { } if (push != null && push.isBidirectional()) { + messageQueue.clear(); // When using bidirectional transport, the payload is not resent // to the server during reconnection attempts. // Keep a copy of the message, so that it could be resent to the @@ -377,7 +378,7 @@ public void setClientToServerMessageId(int nextExpectedId, boolean force) { ApplicationConstants.CLIENT_TO_SERVER_ID) < nextExpectedId) { pushPendingMessage = null; } - if (!messageQueue.isEmpty()) { + if (hasQueuedMessages()) { synchronized (messageQueue) { // If queued message is the expected one. remove from queue // and sen next message if any. From aefbb0aae3dd8f7d83edff2645f021247deef1d6 Mon Sep 17 00:00:00 2001 From: Mikael Grankvist Date: Fri, 10 Jan 2025 13:46:06 +0200 Subject: [PATCH 6/6] Do not up clientId for message already containing clientId --- .../com/vaadin/client/communication/MessageSender.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java b/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java index 6e2e3921643..49a48f252cd 100644 --- a/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java +++ b/flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java @@ -228,8 +228,11 @@ public void send(final JsonObject payload) { private void sendPayload(final JsonObject payload) { payload.put(ApplicationConstants.SERVER_SYNC_ID, registry.getMessageHandler().getLastSeenServerSyncId()); - payload.put(ApplicationConstants.CLIENT_TO_SERVER_ID, - clientToServerMessageId++); + if (!payload.hasKey(ApplicationConstants.CLIENT_TO_SERVER_ID)) { + // We are resending the message so we should not up the clientId + payload.put(ApplicationConstants.CLIENT_TO_SERVER_ID, + clientToServerMessageId++); + } if (!registry.getRequestResponseTracker().hasActiveRequest()) { // Direct calls to send from outside probably have not started @@ -238,7 +241,6 @@ private void sendPayload(final JsonObject payload) { } if (push != null && push.isBidirectional()) { - messageQueue.clear(); // When using bidirectional transport, the payload is not resent // to the server during reconnection attempts. // Keep a copy of the message, so that it could be resent to the