Fix: move blocking calls outside session lock (#20475) (CP:24.4)

flow-server/src/main/java/com/vaadin/flow/server/SynchronizedRequestHandler.java

@@ -15,7 +15,11 @@
  */
 package com.vaadin.flow.server;
 
+import java.io.BufferedReader;
 import java.io.IOException;
+import java.io.Reader;
+import java.io.Serializable;
+import java.util.Optional;
 
 /**
  * RequestHandler which takes care of locking and unlocking of the VaadinSession
@@ -28,18 +32,49 @@
  */
 public abstract class SynchronizedRequestHandler implements RequestHandler {
 
+    public static final int MAX_BUFFER_SIZE = 64 * 1024;
+
+    /**
+     * ResponseWriter is optionally returned by request handlers which implement
+     * {@link SynchronizedRequestHandler#synchronizedHandleRequest(VaadinSession, VaadinRequest, VaadinResponse, String)}
+     *
+     * The ResponseWriter will be executed by
+     * {@link #handleRequest(VaadinSession, VaadinRequest, VaadinResponse)}
+     * without holding Vaadin session lock.
+     */
+    @FunctionalInterface
+    public interface ResponseWriter extends Serializable {
+        void writeResponse() throws IOException;
+    }
+
     @Override
     public boolean handleRequest(VaadinSession session, VaadinRequest request,
             VaadinResponse response) throws IOException {
         if (!canHandleRequest(request)) {
             return false;
         }
 
-        session.lock();
         try {
-            return synchronizedHandleRequest(session, request, response);
+            if (isReadAndWriteOutsideSessionLock()) {
+                BufferedReader reader = request.getReader();
+                String requestBody = reader == null ? null
+                        : getRequestBody(reader);
+                session.lock();
+                Optional<ResponseWriter> responseWriter = synchronizedHandleRequest(
+                        session, request, response, requestBody);
+                session.unlock();
+                if (responseWriter.isPresent()) {
+                    responseWriter.get().writeResponse();
+                }
+                return responseWriter.isPresent();
+            } else {
+                session.lock();
+                return synchronizedHandleRequest(session, request, response);
+            }
         } finally {
-            session.unlock();
+            if (session.hasLock()) {
+                session.unlock();
+            }
         }
     }
 
@@ -65,6 +100,51 @@ public boolean handleRequest(VaadinSession session, VaadinRequest request,
     public abstract boolean synchronizedHandleRequest(VaadinSession session,
             VaadinRequest request, VaadinResponse response) throws IOException;
 
+    /**
+     * Gets if request body should be read and the response written without
+     * holding {@link VaadinSession} lock
+     *
+     * @return {@literal true} if
+     *         {@link #synchronizedHandleRequest(VaadinSession, VaadinRequest, VaadinResponse, String)}
+     *         should be called. Returns {@literal false} if
+     *         {@link #synchronizedHandleRequest(VaadinSession, VaadinRequest, VaadinResponse)}
+     *         should be called.
+     */
+    public boolean isReadAndWriteOutsideSessionLock() {
+        return false;
+    }
+
+    /**
+     * Identical to
+     * {@link #synchronizedHandleRequest(VaadinSession, VaadinRequest, VaadinResponse)}
+     * except the {@link VaadinSession} is locked before this is called and the
+     * response requestBody has been read before locking the session and is
+     * provided as a separate parameter.
+     *
+     * @param session
+     *            The session for the request
+     * @param request
+     *            The request to handle
+     * @param response
+     *            The response object to which a response can be written.
+     * @param requestBody
+     *            Request body pre-read from the request object
+     * @return a ResponseWriter wrapped into an Optional, if this handler will
+     *         write the response and no further request handlers should be
+     *         called, otherwise an empty Optional. The ResponseWriter will be
+     *         executed after the VaadinSession is unlocked.
+     *
+     * @throws IOException
+     *             If an IO error occurred
+     * @see #handleRequest(VaadinSession, VaadinRequest, VaadinResponse)
+     */
+    public Optional<ResponseWriter> synchronizedHandleRequest(
+            VaadinSession session, VaadinRequest request,
+            VaadinResponse response, String requestBody)
+            throws IOException, UnsupportedOperationException {
+        throw new UnsupportedOperationException();
+    }
+
     /**
      * Check whether a request may be handled by this handler. This can be used
      * as an optimization to avoid locking the session just to investigate some
@@ -85,4 +165,18 @@ protected boolean canHandleRequest(VaadinRequest request) {
         return true;
     }
 
+    public static String getRequestBody(Reader reader) throws IOException {
+        StringBuilder sb = new StringBuilder(MAX_BUFFER_SIZE);
+        char[] buffer = new char[MAX_BUFFER_SIZE];
+
+        while (true) {
+            int read = reader.read(buffer);
+            if (read == -1) {
+                break;
+            }
+            sb.append(buffer, 0, read);
+        }
+
+        return sb.toString();
+    }
 }

flow-server/src/main/java/com/vaadin/flow/server/communication/PushHandler.java

@@ -42,6 +42,7 @@
 import com.vaadin.flow.server.ErrorEvent;
 import com.vaadin.flow.server.HandlerHelper;
 import com.vaadin.flow.server.SessionExpiredException;
+import com.vaadin.flow.server.SynchronizedRequestHandler;
 import com.vaadin.flow.server.SystemMessages;
 import com.vaadin.flow.server.VaadinContext;
 import com.vaadin.flow.server.VaadinRequest;
@@ -54,7 +55,6 @@
 import com.vaadin.flow.shared.ApplicationConstants;
 import com.vaadin.flow.shared.JsonConstants;
 import com.vaadin.flow.shared.communication.PushMode;
-
 import elemental.json.JsonException;
 
 /**
@@ -162,7 +162,9 @@ interface PushEventCallback {
         assert vaadinRequest != null;
 
         try {
-            new ServerRpcHandler().handleRpc(ui, reader, vaadinRequest);
+            new ServerRpcHandler().handleRpc(ui,
+                    SynchronizedRequestHandler.getRequestBody(reader),
+                    vaadinRequest);
             connection.push(false);
         } catch (JsonException e) {
             getLogger().error("Error writing JSON to response", e);

flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java

@@ -37,6 +37,7 @@
 import com.vaadin.flow.internal.StateNode;
 import com.vaadin.flow.router.PreserveOnRefresh;
 import com.vaadin.flow.server.ErrorEvent;
+import com.vaadin.flow.server.SynchronizedRequestHandler;
 import com.vaadin.flow.server.VaadinRequest;
 import com.vaadin.flow.server.VaadinService;
 import com.vaadin.flow.server.communication.rpc.AttachExistingElementRpcHandler;
@@ -90,6 +91,11 @@ public static class RpcRequest implements Serializable {
          *            the request through which the JSON was received
          */
         public RpcRequest(String jsonString, VaadinRequest request) {
+            this(jsonString, request.getService().getDeploymentConfiguration()
+                    .isSyncIdCheckEnabled());
+        }
+
+        public RpcRequest(String jsonString, boolean isSyncIdCheckEnabled) {
             json = JsonUtil.parse(jsonString);
 
             JsonValue token = json.get(ApplicationConstants.CSRF_TOKEN);
@@ -103,8 +109,7 @@ public RpcRequest(String jsonString, VaadinRequest request) {
                 this.csrfToken = csrfToken;
             }
 
-            if (request.getService().getDeploymentConfiguration()
-                    .isSyncIdCheckEnabled()) {
+            if (isSyncIdCheckEnabled) {
                 syncId = (int) json
                         .getNumber(ApplicationConstants.SERVER_SYNC_ID);
             } else {
@@ -196,8 +201,6 @@ private boolean isUnloadBeaconRequest() {
 
     }
 
-    private static final int MAX_BUFFER_SIZE = 64 * 1024;
-
     /**
      * Exception thrown then the security key sent by the client does not match
      * the expected one.
@@ -248,26 +251,45 @@ public ResynchronizationRequiredException() {
      */
     public void handleRpc(UI ui, Reader reader, VaadinRequest request)
             throws IOException, InvalidUIDLSecurityKeyException {
-        ui.getSession().setLastRequestTimestamp(System.currentTimeMillis());
+        handleRpc(ui, SynchronizedRequestHandler.getRequestBody(reader),
+                request);
+    }
 
-        String changeMessage = getMessage(reader);
+    /**
+     * Reads JSON containing zero or more serialized RPC calls (including legacy
+     * variable changes) and executes the calls.
+     *
+     * @param ui
+     *            The {@link UI} receiving the calls. Cannot be null.
+     * @param message
+     *            The JSON message from the request.
+     * @param request
+     *            The request through which the RPC was received
+     * @throws InvalidUIDLSecurityKeyException
+     *             If the received security key does not match the one stored in
+     *             the session.
+     */
+    public void handleRpc(UI ui, String message, VaadinRequest request)
+            throws InvalidUIDLSecurityKeyException {
+        ui.getSession().setLastRequestTimestamp(System.currentTimeMillis());
 
-        if (changeMessage == null || changeMessage.equals("")) {
+        if (message == null || message.isEmpty()) {
             // The client sometimes sends empty messages, this is probably a bug
             return;
         }
 
-        RpcRequest rpcRequest = new RpcRequest(changeMessage, request);
+        RpcRequest rpcRequest = new RpcRequest(message, request.getService()
+                .getDeploymentConfiguration().isSyncIdCheckEnabled());
 
         // Security: double cookie submission pattern unless disabled by
         // property
         if (!VaadinService.isCsrfTokenValid(ui, rpcRequest.getCsrfToken())) {
             throw new InvalidUIDLSecurityKeyException();
         }
 
-        String hashMessage = changeMessage;
+        String hashMessage = message;
         if (hashMessage.length() > 64 * 1024) {
-            hashMessage = changeMessage.substring(0, 64 * 1024);
+            hashMessage = message.substring(0, 64 * 1024);
         }
         byte[] messageHash = MessageDigestUtil.sha256(hashMessage);
 
@@ -370,7 +392,6 @@ public void handleRpc(UI ui, Reader reader, VaadinRequest request)
                 getLogger().debug("UI closed with a beacon request");
             }
         }
-
     }
 
     // Kind of same as in AbstractNavigationStateRenderer, but gets
@@ -504,8 +525,9 @@ private static void callErrorHandler(UI ui, JsonObject invocationJson,
 
     protected String getMessage(Reader reader) throws IOException {
 
-        StringBuilder sb = new StringBuilder(MAX_BUFFER_SIZE);
-        char[] buffer = new char[MAX_BUFFER_SIZE];
+        StringBuilder sb = new StringBuilder(
+                SynchronizedRequestHandler.MAX_BUFFER_SIZE);
+        char[] buffer = new char[SynchronizedRequestHandler.MAX_BUFFER_SIZE];
 
         while (true) {
             int read = reader.read(buffer);

flow-server/src/main/java/com/vaadin/flow/server/communication/UidlRequestHandler.java

@@ -20,6 +20,7 @@
 import java.io.OutputStream;
 import java.io.StringWriter;
 import java.io.Writer;
+import java.util.Optional;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -99,40 +100,57 @@ protected ServerRpcHandler createRpcHandler() {
     @Override
     public boolean synchronizedHandleRequest(VaadinSession session,
             VaadinRequest request, VaadinResponse response) throws IOException {
+        String requestBody = SynchronizedRequestHandler
+                .getRequestBody(request.getReader());
+        Optional<ResponseWriter> responseWriter = synchronizedHandleRequest(
+                session, request, response, requestBody);
+        if (responseWriter.isPresent()) {
+            responseWriter.get().writeResponse();
+        }
+        return responseWriter.isPresent();
+    }
+
+    @Override
+    public boolean isReadAndWriteOutsideSessionLock() {
+        return true;
+    }
+
+    @Override
+    public Optional<ResponseWriter> synchronizedHandleRequest(
+            VaadinSession session, VaadinRequest request,
+            VaadinResponse response, String requestBody)
+            throws IOException, UnsupportedOperationException {
         UI uI = session.getService().findUI(request);
         if (uI == null) {
             // This should not happen but it will if the UI has been closed. We
             // really don't want to see it in the server logs though
-            commitJsonResponse(response,
-                    VaadinService.createUINotFoundJSON(false));
-            return true;
+            return Optional.of(() -> commitJsonResponse(response,
+                    VaadinService.createUINotFoundJSON(false)));
         }
 
         StringWriter stringWriter = new StringWriter();
 
         try {
-            getRpcHandler(session).handleRpc(uI, request.getReader(), request);
+            getRpcHandler(session).handleRpc(uI, requestBody, request);
             writeUidl(uI, stringWriter, false);
         } catch (JsonException e) {
             getLogger().error("Error writing JSON to response", e);
             // Refresh on client side
-            writeRefresh(response);
-            return true;
+            return Optional.of(() -> writeRefresh(response));
         } catch (InvalidUIDLSecurityKeyException e) {
             getLogger().warn("Invalid security key received from {}",
                     request.getRemoteHost());
             // Refresh on client side
-            writeRefresh(response);
-            return true;
+            return Optional.of(() -> writeRefresh(response));
         } catch (ResynchronizationRequiredException e) { // NOSONAR
             // Resync on the client side
             writeUidl(uI, stringWriter, true);
         } finally {
             stringWriter.close();
         }
 
-        commitJsonResponse(response, stringWriter.toString());
-        return true;
+        return Optional.of(
+                () -> commitJsonResponse(response, stringWriter.toString()));
     }
 
     private void writeRefresh(VaadinResponse response) throws IOException {

flow-server/src/test/java/com/vaadin/flow/server/communication/ServerRpcHandlerTest.java

@@ -1,7 +1,6 @@
 package com.vaadin.flow.server.communication;
 
 import java.io.IOException;
-import java.io.Reader;
 import java.io.StringReader;
 
 import org.junit.Assert;
@@ -98,36 +97,26 @@ public void handleRpc_resynchronize_throwsExceptionAndDirtiesTreeAndClearsDepend
     public void handleRpc_duplicateMessage_doNotThrow()
             throws InvalidUIDLSecurityKeyException, IOException {
         String msg = "{\"" + ApplicationConstants.CLIENT_TO_SERVER_ID + "\":1}";
-        ServerRpcHandler handler = new ServerRpcHandler() {
-            @Override
-            protected String getMessage(Reader reader) throws IOException {
-                return msg;
-            };
-        };
+        ServerRpcHandler handler = new ServerRpcHandler();
 
         ui = new UI();
         ui.getInternals().setSession(session);
         ui.getInternals().setLastProcessedClientToServerId(1,
                 MessageDigestUtil.sha256(msg));
 
         // This invocation shouldn't throw. No other checks
-        handler.handleRpc(ui, Mockito.mock(Reader.class), request);
+        handler.handleRpc(ui, msg, request);
     }
 
     @Test(expected = UnsupportedOperationException.class)
     public void handleRpc_unexpectedMessage_throw()
             throws InvalidUIDLSecurityKeyException, IOException {
-        ServerRpcHandler handler = new ServerRpcHandler() {
-            @Override
-            protected String getMessage(Reader reader) throws IOException {
-                return "{\"" + ApplicationConstants.CLIENT_TO_SERVER_ID
-                        + "\":1}";
-            };
-        };
+        String msg = "{\"" + ApplicationConstants.CLIENT_TO_SERVER_ID + "\":1}";
+        ServerRpcHandler handler = new ServerRpcHandler();
 
         ui = new UI();
         ui.getInternals().setSession(session);
 
-        handler.handleRpc(ui, Mockito.mock(Reader.class), request);
+        handler.handleRpc(ui, msg, request);
     }
 }