Skip to content

Commit f116da4

Browse files
tltvtltv
authored
Fix: move blocking calls outside session lock (#19938) (#20475) (#21972)
1 parent bd21ce7 commit f116da4

File tree

6 files changed

+176
-47
lines changed

6 files changed

+176
-47
lines changed

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

Lines changed: 97 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@
88
*/
99
package com.vaadin.flow.server;
1010

11+
import java.io.BufferedReader;
1112
import java.io.IOException;
13+
import java.io.Reader;
14+
import java.io.Serializable;
15+
import java.util.Optional;
1216

1317
/**
1418
* RequestHandler which takes care of locking and unlocking of the VaadinSession
@@ -21,18 +25,49 @@
2125
*/
2226
public abstract class SynchronizedRequestHandler implements RequestHandler {
2327

28+
public static final int MAX_BUFFER_SIZE = 64 * 1024;
29+
30+
/**
31+
* ResponseWriter is optionally returned by request handlers which implement
32+
* {@link SynchronizedRequestHandler#synchronizedHandleRequest(VaadinSession, VaadinRequest, VaadinResponse, String)}
33+
*
34+
* The ResponseWriter will be executed by
35+
* {@link #handleRequest(VaadinSession, VaadinRequest, VaadinResponse)}
36+
* without holding Vaadin session lock.
37+
*/
38+
@FunctionalInterface
39+
public interface ResponseWriter extends Serializable {
40+
void writeResponse() throws IOException;
41+
}
42+
2443
@Override
2544
public boolean handleRequest(VaadinSession session, VaadinRequest request,
2645
VaadinResponse response) throws IOException {
2746
if (!canHandleRequest(request)) {
2847
return false;
2948
}
3049

31-
session.lock();
3250
try {
33-
return synchronizedHandleRequest(session, request, response);
51+
if (isReadAndWriteOutsideSessionLock()) {
52+
BufferedReader reader = request.getReader();
53+
String requestBody = reader == null ? null
54+
: getRequestBody(reader);
55+
session.lock();
56+
Optional<ResponseWriter> responseWriter = synchronizedHandleRequest(
57+
session, request, response, requestBody);
58+
session.unlock();
59+
if (responseWriter.isPresent()) {
60+
responseWriter.get().writeResponse();
61+
}
62+
return responseWriter.isPresent();
63+
} else {
64+
session.lock();
65+
return synchronizedHandleRequest(session, request, response);
66+
}
3467
} finally {
35-
session.unlock();
68+
if (session.hasLock()) {
69+
session.unlock();
70+
}
3671
}
3772
}
3873

@@ -58,6 +93,51 @@ public boolean handleRequest(VaadinSession session, VaadinRequest request,
5893
public abstract boolean synchronizedHandleRequest(VaadinSession session,
5994
VaadinRequest request, VaadinResponse response) throws IOException;
6095

96+
/**
97+
* Gets if request body should be read and the response written without
98+
* holding {@link VaadinSession} lock
99+
*
100+
* @return {@literal true} if
101+
* {@link #synchronizedHandleRequest(VaadinSession, VaadinRequest, VaadinResponse, String)}
102+
* should be called. Returns {@literal false} if
103+
* {@link #synchronizedHandleRequest(VaadinSession, VaadinRequest, VaadinResponse)}
104+
* should be called.
105+
*/
106+
public boolean isReadAndWriteOutsideSessionLock() {
107+
return false;
108+
}
109+
110+
/**
111+
* Identical to
112+
* {@link #synchronizedHandleRequest(VaadinSession, VaadinRequest, VaadinResponse)}
113+
* except the {@link VaadinSession} is locked before this is called and the
114+
* response requestBody has been read before locking the session and is
115+
* provided as a separate parameter.
116+
*
117+
* @param session
118+
* The session for the request
119+
* @param request
120+
* The request to handle
121+
* @param response
122+
* The response object to which a response can be written.
123+
* @param requestBody
124+
* Request body pre-read from the request object
125+
* @return a ResponseWriter wrapped into an Optional, if this handler will
126+
* write the response and no further request handlers should be
127+
* called, otherwise an empty Optional. The ResponseWriter will be
128+
* executed after the VaadinSession is unlocked.
129+
*
130+
* @throws IOException
131+
* If an IO error occurred
132+
* @see #handleRequest(VaadinSession, VaadinRequest, VaadinResponse)
133+
*/
134+
public Optional<ResponseWriter> synchronizedHandleRequest(
135+
VaadinSession session, VaadinRequest request,
136+
VaadinResponse response, String requestBody)
137+
throws IOException, UnsupportedOperationException {
138+
throw new UnsupportedOperationException();
139+
}
140+
61141
/**
62142
* Check whether a request may be handled by this handler. This can be used
63143
* as an optimization to avoid locking the session just to investigate some
@@ -78,4 +158,18 @@ protected boolean canHandleRequest(VaadinRequest request) {
78158
return true;
79159
}
80160

161+
public static String getRequestBody(Reader reader) throws IOException {
162+
StringBuilder sb = new StringBuilder(MAX_BUFFER_SIZE);
163+
char[] buffer = new char[MAX_BUFFER_SIZE];
164+
165+
while (true) {
166+
int read = reader.read(buffer);
167+
if (read == -1) {
168+
break;
169+
}
170+
sb.append(buffer, 0, read);
171+
}
172+
173+
return sb.toString();
174+
}
81175
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import com.vaadin.flow.server.ErrorEvent;
3333
import com.vaadin.flow.server.HandlerHelper;
3434
import com.vaadin.flow.server.SessionExpiredException;
35+
import com.vaadin.flow.server.SynchronizedRequestHandler;
3536
import com.vaadin.flow.server.SystemMessages;
3637
import com.vaadin.flow.server.VaadinContext;
3738
import com.vaadin.flow.server.VaadinRequest;
@@ -44,7 +45,6 @@
4445
import com.vaadin.flow.shared.ApplicationConstants;
4546
import com.vaadin.flow.shared.JsonConstants;
4647
import com.vaadin.flow.shared.communication.PushMode;
47-
4848
import elemental.json.JsonException;
4949

5050
/**
@@ -144,7 +144,9 @@ interface PushEventCallback {
144144
assert vaadinRequest != null;
145145

146146
try {
147-
new ServerRpcHandler().handleRpc(ui, reader, vaadinRequest);
147+
new ServerRpcHandler().handleRpc(ui,
148+
SynchronizedRequestHandler.getRequestBody(reader),
149+
vaadinRequest);
148150
connection.push(false);
149151
} catch (JsonException e) {
150152
getLogger().error("Error writing JSON to response", e);

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

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import com.vaadin.flow.internal.MessageDigestUtil;
3030
import com.vaadin.flow.router.PreserveOnRefresh;
3131
import com.vaadin.flow.server.ErrorEvent;
32+
import com.vaadin.flow.server.SynchronizedRequestHandler;
3233
import com.vaadin.flow.server.VaadinRequest;
3334
import com.vaadin.flow.server.VaadinService;
3435
import com.vaadin.flow.server.communication.rpc.AttachExistingElementRpcHandler;
@@ -82,6 +83,11 @@ public static class RpcRequest implements Serializable {
8283
* the request through which the JSON was received
8384
*/
8485
public RpcRequest(String jsonString, VaadinRequest request) {
86+
this(jsonString, request.getService().getDeploymentConfiguration()
87+
.isSyncIdCheckEnabled());
88+
}
89+
90+
public RpcRequest(String jsonString, boolean isSyncIdCheckEnabled) {
8591
json = JsonUtil.parse(jsonString);
8692

8793
JsonValue token = json.get(ApplicationConstants.CSRF_TOKEN);
@@ -95,8 +101,7 @@ public RpcRequest(String jsonString, VaadinRequest request) {
95101
this.csrfToken = csrfToken;
96102
}
97103

98-
if (request.getService().getDeploymentConfiguration()
99-
.isSyncIdCheckEnabled()) {
104+
if (isSyncIdCheckEnabled) {
100105
syncId = (int) json
101106
.getNumber(ApplicationConstants.SERVER_SYNC_ID);
102107
} else {
@@ -188,8 +193,6 @@ private boolean isUnloadBeaconRequest() {
188193

189194
}
190195

191-
private static final int MAX_BUFFER_SIZE = 64 * 1024;
192-
193196
/**
194197
* Exception thrown then the security key sent by the client does not match
195198
* the expected one.
@@ -240,26 +243,45 @@ public ResynchronizationRequiredException() {
240243
*/
241244
public void handleRpc(UI ui, Reader reader, VaadinRequest request)
242245
throws IOException, InvalidUIDLSecurityKeyException {
243-
ui.getSession().setLastRequestTimestamp(System.currentTimeMillis());
246+
handleRpc(ui, SynchronizedRequestHandler.getRequestBody(reader),
247+
request);
248+
}
244249

245-
String changeMessage = getMessage(reader);
250+
/**
251+
* Reads JSON containing zero or more serialized RPC calls (including legacy
252+
* variable changes) and executes the calls.
253+
*
254+
* @param ui
255+
* The {@link UI} receiving the calls. Cannot be null.
256+
* @param message
257+
* The JSON message from the request.
258+
* @param request
259+
* The request through which the RPC was received
260+
* @throws InvalidUIDLSecurityKeyException
261+
* If the received security key does not match the one stored in
262+
* the session.
263+
*/
264+
public void handleRpc(UI ui, String message, VaadinRequest request)
265+
throws InvalidUIDLSecurityKeyException {
266+
ui.getSession().setLastRequestTimestamp(System.currentTimeMillis());
246267

247-
if (changeMessage == null || changeMessage.equals("")) {
268+
if (message == null || message.isEmpty()) {
248269
// The client sometimes sends empty messages, this is probably a bug
249270
return;
250271
}
251272

252-
RpcRequest rpcRequest = new RpcRequest(changeMessage, request);
273+
RpcRequest rpcRequest = new RpcRequest(message, request.getService()
274+
.getDeploymentConfiguration().isSyncIdCheckEnabled());
253275

254276
// Security: double cookie submission pattern unless disabled by
255277
// property
256278
if (!VaadinService.isCsrfTokenValid(ui, rpcRequest.getCsrfToken())) {
257279
throw new InvalidUIDLSecurityKeyException();
258280
}
259281

260-
String hashMessage = changeMessage;
282+
String hashMessage = message;
261283
if (hashMessage.length() > 64 * 1024) {
262-
hashMessage = changeMessage.substring(0, 64 * 1024);
284+
hashMessage = message.substring(0, 64 * 1024);
263285
}
264286
byte[] messageHash = MessageDigestUtil.sha256(hashMessage);
265287

@@ -361,7 +383,6 @@ public void handleRpc(UI ui, Reader reader, VaadinRequest request)
361383
getLogger().debug("UI closed with a beacon request");
362384
}
363385
}
364-
365386
}
366387

367388
// Kind of same as in AbstractNavigationStateRenderer, but gets
@@ -476,8 +497,9 @@ private void handleInvocationData(UI ui, JsonObject invocationJson) {
476497

477498
protected String getMessage(Reader reader) throws IOException {
478499

479-
StringBuilder sb = new StringBuilder(MAX_BUFFER_SIZE);
480-
char[] buffer = new char[MAX_BUFFER_SIZE];
500+
StringBuilder sb = new StringBuilder(
501+
SynchronizedRequestHandler.MAX_BUFFER_SIZE);
502+
char[] buffer = new char[SynchronizedRequestHandler.MAX_BUFFER_SIZE];
481503

482504
while (true) {
483505
int read = reader.read(buffer);

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

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import java.io.OutputStream;
1313
import java.io.StringWriter;
1414
import java.io.Writer;
15+
import java.util.Optional;
1516
import java.util.concurrent.atomic.AtomicReference;
1617
import java.util.regex.Matcher;
1718
import java.util.regex.Pattern;
@@ -92,40 +93,57 @@ protected ServerRpcHandler createRpcHandler() {
9293
@Override
9394
public boolean synchronizedHandleRequest(VaadinSession session,
9495
VaadinRequest request, VaadinResponse response) throws IOException {
96+
String requestBody = SynchronizedRequestHandler
97+
.getRequestBody(request.getReader());
98+
Optional<ResponseWriter> responseWriter = synchronizedHandleRequest(
99+
session, request, response, requestBody);
100+
if (responseWriter.isPresent()) {
101+
responseWriter.get().writeResponse();
102+
}
103+
return responseWriter.isPresent();
104+
}
105+
106+
@Override
107+
public boolean isReadAndWriteOutsideSessionLock() {
108+
return true;
109+
}
110+
111+
@Override
112+
public Optional<ResponseWriter> synchronizedHandleRequest(
113+
VaadinSession session, VaadinRequest request,
114+
VaadinResponse response, String requestBody)
115+
throws IOException, UnsupportedOperationException {
95116
UI uI = session.getService().findUI(request);
96117
if (uI == null) {
97118
// This should not happen but it will if the UI has been closed. We
98119
// really don't want to see it in the server logs though
99-
commitJsonResponse(response,
100-
VaadinService.createUINotFoundJSON(false));
101-
return true;
120+
return Optional.of(() -> commitJsonResponse(response,
121+
VaadinService.createUINotFoundJSON(false)));
102122
}
103123

104124
StringWriter stringWriter = new StringWriter();
105125

106126
try {
107-
getRpcHandler(session).handleRpc(uI, request.getReader(), request);
127+
getRpcHandler(session).handleRpc(uI, requestBody, request);
108128
writeUidl(uI, stringWriter, false);
109129
} catch (JsonException e) {
110130
getLogger().error("Error writing JSON to response", e);
111131
// Refresh on client side
112-
writeRefresh(response);
113-
return true;
132+
return Optional.of(() -> writeRefresh(response));
114133
} catch (InvalidUIDLSecurityKeyException e) {
115134
getLogger().warn("Invalid security key received from {}",
116135
request.getRemoteHost());
117136
// Refresh on client side
118-
writeRefresh(response);
119-
return true;
137+
return Optional.of(() -> writeRefresh(response));
120138
} catch (ResynchronizationRequiredException e) { // NOSONAR
121139
// Resync on the client side
122140
writeUidl(uI, stringWriter, true);
123141
} finally {
124142
stringWriter.close();
125143
}
126144

127-
commitJsonResponse(response, stringWriter.toString());
128-
return true;
145+
return Optional.of(
146+
() -> commitJsonResponse(response, stringWriter.toString()));
129147
}
130148

131149
private void writeRefresh(VaadinResponse response) throws IOException {

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

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -106,37 +106,27 @@ public void handleRpc_resynchronize_throwsExceptionAndDirtiesTreeAndClearsDepend
106106
public void handleRpc_duplicateMessage_doNotThrow()
107107
throws InvalidUIDLSecurityKeyException, IOException {
108108
String msg = "{\"" + ApplicationConstants.CLIENT_TO_SERVER_ID + "\":1}";
109-
ServerRpcHandler handler = new ServerRpcHandler() {
110-
@Override
111-
protected String getMessage(Reader reader) throws IOException {
112-
return msg;
113-
};
114-
};
109+
ServerRpcHandler handler = new ServerRpcHandler();
115110

116111
ui = new UI();
117112
ui.getInternals().setSession(session);
118113
ui.getInternals().setLastProcessedClientToServerId(1,
119114
MessageDigestUtil.sha256(msg));
120115

121116
// This invocation shouldn't throw. No other checks
122-
handler.handleRpc(ui, Mockito.mock(Reader.class), request);
117+
handler.handleRpc(ui, msg, request);
123118
}
124119

125120
@Test(expected = UnsupportedOperationException.class)
126121
public void handleRpc_unexpectedMessage_throw()
127122
throws InvalidUIDLSecurityKeyException, IOException {
128-
ServerRpcHandler handler = new ServerRpcHandler() {
129-
@Override
130-
protected String getMessage(Reader reader) throws IOException {
131-
return "{\"" + ApplicationConstants.CLIENT_TO_SERVER_ID
132-
+ "\":1}";
133-
};
134-
};
123+
String msg = "{\"" + ApplicationConstants.CLIENT_TO_SERVER_ID + "\":1}";
124+
ServerRpcHandler handler = new ServerRpcHandler();
135125

136126
ui = new UI();
137127
ui.getInternals().setSession(session);
138128

139-
handler.handleRpc(ui, Mockito.mock(Reader.class), request);
129+
handler.handleRpc(ui, msg, request);
140130
}
141131

142132
@Test

0 commit comments

Comments
 (0)