From 23093d75687a6b8600b28dac3509f1140b28da3e Mon Sep 17 00:00:00 2001 From: Teppo Kurki Date: Tue, 19 Nov 2024 11:11:44 +0200 Subject: [PATCH] Fix: move blocking calls outside session lock (#19938) (#20475) Move blocking calls outside session lock (#19938) --- .../server/SynchronizedRequestHandler.java | 100 +++++++++++++++++- .../server/communication/PushHandler.java | 6 +- .../communication/ServerRpcHandler.java | 48 ++++++--- .../communication/UidlRequestHandler.java | 41 ++++--- .../communication/ServerRpcHandlerTest.java | 24 +---- .../communication/UidlRequestHandlerTest.java | 16 +-- 6 files changed, 180 insertions(+), 55 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/SynchronizedRequestHandler.java b/flow-server/src/main/java/com/vaadin/flow/server/SynchronizedRequestHandler.java index d271b8a189f..a5432b00587 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/SynchronizedRequestHandler.java +++ b/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,6 +32,21 @@ */ 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 { @@ -35,11 +54,27 @@ public boolean handleRequest(VaadinSession session, VaadinRequest 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 = 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 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(); + } } diff --git a/flow-server/src/main/java/com/vaadin/flow/server/communication/PushHandler.java b/flow-server/src/main/java/com/vaadin/flow/server/communication/PushHandler.java index 9af8e65f458..06b0965b91f 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/communication/PushHandler.java +++ b/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; @@ -56,7 +57,6 @@ import com.vaadin.flow.shared.ApplicationConstants; import com.vaadin.flow.shared.JsonConstants; import com.vaadin.flow.shared.communication.PushMode; - import elemental.json.JsonException; /** @@ -164,7 +164,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); 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 ff8b1ad47d7..291d887d4c3 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 @@ -39,6 +39,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; @@ -94,6 +95,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); @@ -107,8 +113,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 { @@ -199,8 +204,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. @@ -251,16 +254,35 @@ 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 @@ -268,9 +290,9 @@ public void handleRpc(UI ui, Reader reader, VaadinRequest request) 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); @@ -374,7 +396,6 @@ public void handleRpc(UI ui, Reader reader, VaadinRequest request) getLogger().debug("UI closed with a beacon request"); } } - } private void enforceIfNeeded(VaadinRequest request, RpcRequest rpcRequest) { @@ -550,8 +571,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); diff --git a/flow-server/src/main/java/com/vaadin/flow/server/communication/UidlRequestHandler.java b/flow-server/src/main/java/com/vaadin/flow/server/communication/UidlRequestHandler.java index 4a1b75f2f1f..8a90334aac1 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/communication/UidlRequestHandler.java +++ b/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; @@ -102,39 +103,55 @@ protected ServerRpcHandler createRpcHandler() { @Override public boolean synchronizedHandleRequest(VaadinSession session, VaadinRequest request, VaadinResponse response) throws IOException { + String requestBody = SynchronizedRequestHandler + .getRequestBody(request.getReader()); + Optional responseWriter = synchronizedHandleRequest( + session, request, response, requestBody); + if (responseWriter.isPresent()) { + responseWriter.get().writeResponse(); + } + return responseWriter.isPresent(); + } + + @Override + public boolean isReadAndWriteOutsideSessionLock() { + return true; + } + + @Override + public Optional 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 (DauEnforcementException e) { getLogger().warn( "Daily Active User limit reached. Blocking new user request"); response.setHeader(DAUUtils.STATUS_CODE_KEY, String .valueOf(HttpStatusCode.SERVICE_UNAVAILABLE.getCode())); String json = DAUUtils.jsonEnforcementResponse(request, e); - commitJsonResponse(response, json); - return true; + return Optional.of(() -> commitJsonResponse(response, json)); } catch (ResynchronizationRequiredException e) { // NOSONAR // Resync on the client side writeUidl(uI, stringWriter, true); @@ -142,8 +159,8 @@ public boolean synchronizedHandleRequest(VaadinSession session, stringWriter.close(); } - commitJsonResponse(response, stringWriter.toString()); - return true; + return Optional.of( + () -> commitJsonResponse(response, stringWriter.toString())); } private void writeRefresh(VaadinResponse response) throws IOException { diff --git a/flow-server/src/test/java/com/vaadin/flow/server/communication/ServerRpcHandlerTest.java b/flow-server/src/test/java/com/vaadin/flow/server/communication/ServerRpcHandlerTest.java index 85ac8d956f9..0996a9768ff 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/communication/ServerRpcHandlerTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/communication/ServerRpcHandlerTest.java @@ -103,14 +103,7 @@ 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); @@ -118,26 +111,19 @@ protected String getMessage(Reader reader) throws IOException { 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); } @Test(expected = DauEnforcementException.class) diff --git a/flow-server/src/test/java/com/vaadin/flow/server/communication/UidlRequestHandlerTest.java b/flow-server/src/test/java/com/vaadin/flow/server/communication/UidlRequestHandlerTest.java index 5d079044cfd..07440e1a9ef 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/communication/UidlRequestHandlerTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/communication/UidlRequestHandlerTest.java @@ -22,6 +22,7 @@ import java.io.Reader; import java.io.StringWriter; import java.util.Collections; +import java.util.Optional; import java.util.Properties; import org.junit.Assert; @@ -33,6 +34,7 @@ import com.vaadin.flow.server.DefaultDeploymentConfiguration; import com.vaadin.flow.server.HandlerHelper.RequestType; import com.vaadin.flow.server.MockVaadinContext; +import com.vaadin.flow.server.SynchronizedRequestHandler; import com.vaadin.flow.server.VaadinContext; import com.vaadin.flow.server.VaadinRequest; import com.vaadin.flow.server.VaadinResponse; @@ -112,10 +114,11 @@ public void writeSessionExpired_whenUINotFound() throws IOException { when(service.findUI(request)).thenReturn(null); - boolean result = handler.synchronizedHandleRequest(session, request, - response); - Assert.assertTrue("Result should be true", result); - + Optional result = handler + .synchronizedHandleRequest(session, request, response, null); + Assert.assertTrue("ResponseWriter should be present", + result.isPresent()); + result.get().writeResponse(); String responseContent = CommunicationUtil .getStringWhenWriteString(outputStream); @@ -241,7 +244,8 @@ public void synchronizedHandleRequest_DauEnforcementException_setsStatusCode503( ServerRpcHandler serverRpcHandler = new ServerRpcHandler() { @Override - public void handleRpc(UI ui, Reader reader, VaadinRequest request) { + public void handleRpc(UI ui, String requestBody, + VaadinRequest request) { throw new DauEnforcementException( new EnforcementException("test")); } @@ -254,7 +258,7 @@ protected ServerRpcHandler createRpcHandler() { } }; - handler.synchronizedHandleRequest(session, request, response); + handler.synchronizedHandleRequest(session, request, response, ""); Mockito.verify(response).setHeader(DAUUtils.STATUS_CODE_KEY, "503"); }