Skip to content

Commit

Permalink
Fix: move blocking calls outside session lock (#19938) (#20475)
Browse files Browse the repository at this point in the history 
Move blocking calls outside session lock (#19938)
  • Loading branch information
@tepi
tepi authored Nov 19, 2024
1 parent 368cd1c commit 23093d7
Show file tree
Hide file tree
Showing 6 changed files with 180 additions and 55 deletions.
100 changes: 97 additions & 3 deletions flow-server/src/main/java/com/vaadin/flow/server/SynchronizedRequestHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
}
}
}

Expand All @@ -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
Expand All @@ -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();
}
}
6 changes: 4 additions & 2 deletions flow-server/src/main/java/com/vaadin/flow/server/communication/PushHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

/**
Expand Down Expand Up @@ -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);
Expand Down
48 changes: 35 additions & 13 deletions flow-server/src/main/java/com/vaadin/flow/server/communication/ServerRpcHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -251,26 +254,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);

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down
41 changes: 29 additions & 12 deletions flow-server/src/main/java/com/vaadin/flow/server/communication/UidlRequestHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -102,48 +103,64 @@ 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 (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);
} finally {
stringWriter.close();
}

commitJsonResponse(response, stringWriter.toString());
return true;
return Optional.of(
() -> commitJsonResponse(response, stringWriter.toString()));
}

private void writeRefresh(VaadinResponse response) throws IOException {
Expand Down
Loading

0 comments on commit 23093d7

Please sign in to comment.