Skip to content

Commit 8200601

Browse files
rstoyanchevrstoyanchev
committed
Tighten up exception handling strategy
WebSocketHandler implementations: - methods must deal with exceptions locally - uncaught runtime exceptions are handled by ending the session - transport errors (websocket engine) are passed into handleError WebSocketSession methods may raise IOException SockJS implementation of WebSocketHandler: - delegate SockJS transport errors into handleError - stop runtime exceptions from user WebSocketHandler and end session SockJsServce and TransportHandlers: - raise IOException or TransportErrorException HandshakeHandler: - raise IOException
1 parent 34c9503 commit 8200601

32 files changed

+558
-318
lines changed

‎spring-websocket/src/main/java/org/springframework/sockjs/AbstractSockJsSession.java

Lines changed: 71 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package org.springframework.sockjs;
1818

19+
import java.io.IOException;
1920
import java.net.URI;
2021

2122
import org.apache.commons.logging.Log;
@@ -121,10 +122,15 @@ protected void updateLastActiveTime() {
121122
this.timeLastActive = System.currentTimeMillis();
122123
}
123124

124-
public void delegateConnectionEstablished() throws Exception {
125+
public void delegateConnectionEstablished() {
125126
this.state = State.OPEN;
126127
initHandler();
127-
this.handler.afterConnectionEstablished(this);
128+
try {
129+
this.handler.afterConnectionEstablished(this);
130+
}
131+
catch (Throwable ex) {
132+
tryCloseWithError(ex, null);
133+
}
128134
}
129135

130136
private void initHandler() {
@@ -133,14 +139,61 @@ private void initHandler() {
133139
this.handler = (TextMessageHandler) webSocketHandler;
134140
}
135141

136-
public void delegateMessages(String[] messages) throws Exception {
137-
for (String message : messages) {
138-
this.handler.handleTextMessage(new TextMessage(message), this);
142+
/**
143+
* Close due to unhandled runtime error from WebSocketHandler.
144+
* @param closeStatus TODO
145+
*/
146+
private void tryCloseWithError(Throwable ex, CloseStatus closeStatus) {
147+
logger.error("Unhandled error for " + this, ex);
148+
try {
149+
closeStatus = (closeStatus != null) ? closeStatus : CloseStatus.SERVER_ERROR;
150+
close(closeStatus);
151+
}
152+
catch (Throwable t) {
153+
destroyHandler();
139154
}
140155
}
141156

142-
public void delegateError(Throwable ex) throws Exception {
143-
this.handler.handleError(ex, this);
157+
private void destroyHandler() {
158+
try {
159+
if (this.handler != null) {
160+
this.handlerProvider.destroy(this.handler);
161+
}
162+
}
163+
catch (Throwable t) {
164+
logger.warn("Error while destroying handler", t);
165+
}
166+
finally {
167+
this.handler = null;
168+
}
169+
}
170+
171+
/**
172+
* Close due to error arising from SockJS transport handling.
173+
*/
174+
protected void tryCloseWithSockJsTransportError(Throwable ex, CloseStatus closeStatus) {
175+
delegateError(ex);
176+
tryCloseWithError(ex, closeStatus);
177+
}
178+
179+
public void delegateMessages(String[] messages) {
180+
try {
181+
for (String message : messages) {
182+
this.handler.handleTextMessage(new TextMessage(message), this);
183+
}
184+
}
185+
catch (Throwable ex) {
186+
tryCloseWithError(ex, null);
187+
}
188+
}
189+
190+
public void delegateError(Throwable ex) {
191+
try {
192+
this.handler.handleTransportError(ex, this);
193+
}
194+
catch (Throwable t) {
195+
tryCloseWithError(t, null);
196+
}
144197
}
145198

146199
/**
@@ -149,7 +202,7 @@ public void delegateError(Throwable ex) throws Exception {
149202
* {@link TextMessageHandler}. This is in contrast to {@link #close()} that pro-actively
150203
* closes the connection.
151204
*/
152-
public final void delegateConnectionClosed(CloseStatus status) throws Exception {
205+
public final void delegateConnectionClosed(CloseStatus status) {
153206
if (!isClosed()) {
154207
if (logger.isDebugEnabled()) {
155208
logger.debug(this + " was closed, " + status);
@@ -159,7 +212,12 @@ public final void delegateConnectionClosed(CloseStatus status) throws Exception
159212
}
160213
finally {
161214
this.state = State.CLOSED;
162-
this.handler.afterConnectionClosed(status, this);
215+
try {
216+
this.handler.afterConnectionClosed(status, this);
217+
}
218+
finally {
219+
destroyHandler();
220+
}
163221
}
164222
}
165223
}
@@ -171,15 +229,15 @@ protected void connectionClosedInternal(CloseStatus status) {
171229
* {@inheritDoc}
172230
* <p>Performs cleanup and notifies the {@link SockJsHandler}.
173231
*/
174-
public final void close() throws Exception {
232+
public final void close() throws IOException {
175233
close(CloseStatus.NORMAL);
176234
}
177235

178236
/**
179237
* {@inheritDoc}
180238
* <p>Performs cleanup and notifies the {@link SockJsHandler}.
181239
*/
182-
public final void close(CloseStatus status) throws Exception {
240+
public final void close(CloseStatus status) throws IOException {
183241
if (!isClosed()) {
184242
if (logger.isDebugEnabled()) {
185243
logger.debug("Closing " + this + ", " + status);
@@ -193,13 +251,13 @@ public final void close(CloseStatus status) throws Exception {
193251
this.handler.afterConnectionClosed(status, this);
194252
}
195253
finally {
196-
this.handlerProvider.destroy(this.handler);
254+
destroyHandler();
197255
}
198256
}
199257
}
200258
}
201259

202-
protected abstract void closeInternal(CloseStatus status) throws Exception;
260+
protected abstract void closeInternal(CloseStatus status) throws IOException;
203261

204262

205263
@Override

‎spring-websocket/src/main/java/org/springframework/sockjs/server/AbstractServerSockJsSession.java

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,13 @@ protected SockJsConfiguration getSockJsConfig() {
5656
return this.sockJsConfig;
5757
}
5858

59-
public final synchronized void sendMessage(WebSocketMessage message) throws Exception {
60-
Assert.isTrue(!isClosed(), "Cannot send a message, session has been closed");
59+
public final synchronized void sendMessage(WebSocketMessage message) throws IOException {
60+
Assert.isTrue(!isClosed(), "Cannot send a message when session is closed");
6161
Assert.isInstanceOf(TextMessage.class, message, "Expected text message: " + message);
6262
sendMessageInternal(((TextMessage) message).getPayload());
6363
}
6464

65-
protected abstract void sendMessageInternal(String message) throws Exception;
65+
protected abstract void sendMessageInternal(String message) throws IOException;
6666

6767

6868
@Override
@@ -72,7 +72,7 @@ public void connectionClosedInternal(CloseStatus status) {
7272
}
7373

7474
@Override
75-
public final synchronized void closeInternal(CloseStatus status) throws Exception {
75+
public final synchronized void closeInternal(CloseStatus status) throws IOException {
7676
if (isActive()) {
7777
// TODO: deliver messages "in flight" before sending close frame
7878
try {
@@ -89,13 +89,13 @@ public final synchronized void closeInternal(CloseStatus status) throws Exceptio
8989
}
9090

9191
// TODO: close status/reason
92-
protected abstract void disconnect(CloseStatus status) throws Exception;
92+
protected abstract void disconnect(CloseStatus status) throws IOException;
9393

9494
/**
9595
* For internal use within a TransportHandler and the (TransportHandler-specific)
9696
* session sub-class.
9797
*/
98-
protected void writeFrame(SockJsFrame frame) throws Exception {
98+
protected void writeFrame(SockJsFrame frame) throws IOException {
9999
if (logger.isTraceEnabled()) {
100100
logger.trace("Preparing to write " + frame);
101101
}
@@ -115,7 +115,7 @@ protected void writeFrame(SockJsFrame frame) throws Exception {
115115
catch (Throwable ex) {
116116
logger.warn("Terminating connection due to failure to send message: " + ex.getMessage());
117117
close();
118-
throw new NestedSockJsRuntimeException("Failed to write frame " + frame, ex);
118+
throw new NestedSockJsRuntimeException("Failed to write " + frame, ex);
119119
}
120120
}
121121

@@ -140,7 +140,7 @@ public void run() {
140140
try {
141141
sendHeartbeat();
142142
}
143-
catch (Exception e) {
143+
catch (Throwable t) {
144144
// ignore
145145
}
146146
}

‎spring-websocket/src/main/java/org/springframework/sockjs/server/AbstractSockJsService.java

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,8 @@ public boolean isWebSocketEnabled() {
201201
* @throws Exception
202202
*/
203203
public final void handleRequest(ServerHttpRequest request, ServerHttpResponse response,
204-
String sockJsPath, HandlerProvider<WebSocketHandler> handler) throws Exception {
204+
String sockJsPath, HandlerProvider<WebSocketHandler> handler)
205+
throws IOException, TransportErrorException {
205206

206207
logger.debug(request.getMethod() + " [" + sockJsPath + "]");
207208

@@ -255,10 +256,11 @@ else if (sockJsPath.equals("/websocket")) {
255256
}
256257

257258
protected abstract void handleRawWebSocketRequest(ServerHttpRequest request, ServerHttpResponse response,
258-
HandlerProvider<WebSocketHandler> handler) throws Exception;
259+
HandlerProvider<WebSocketHandler> handler) throws IOException;
259260

260261
protected abstract void handleTransportRequest(ServerHttpRequest request, ServerHttpResponse response,
261-
String sessionId, TransportType transportType, HandlerProvider<WebSocketHandler> handler) throws Exception;
262+
String sessionId, TransportType transportType, HandlerProvider<WebSocketHandler> handler)
263+
throws IOException, TransportErrorException;
262264

263265

264266
protected boolean validateRequest(String serverId, String sessionId, String transport) {
@@ -321,7 +323,7 @@ protected void sendMethodNotAllowed(ServerHttpResponse response, List<HttpMethod
321323

322324
private interface SockJsRequestHandler {
323325

324-
void handle(ServerHttpRequest request, ServerHttpResponse response) throws Exception;
326+
void handle(ServerHttpRequest request, ServerHttpResponse response) throws IOException;
325327
}
326328

327329
private static final Random random = new Random();
@@ -331,7 +333,7 @@ private interface SockJsRequestHandler {
331333
private static final String INFO_CONTENT =
332334
"{\"entropy\":%s,\"origins\":[\"*:*\"],\"cookie_needed\":%s,\"websocket\":%s}";
333335

334-
public void handle(ServerHttpRequest request, ServerHttpResponse response) throws Exception {
336+
public void handle(ServerHttpRequest request, ServerHttpResponse response) throws IOException {
335337

336338
if (HttpMethod.GET.equals(request.getMethod())) {
337339

@@ -376,7 +378,7 @@ else if (HttpMethod.OPTIONS.equals(request.getMethod())) {
376378
"</body>\n" +
377379
"</html>";
378380

379-
public void handle(ServerHttpRequest request, ServerHttpResponse response) throws Exception {
381+
public void handle(ServerHttpRequest request, ServerHttpResponse response) throws IOException {
380382

381383
if (!HttpMethod.GET.equals(request.getMethod())) {
382384
sendMethodNotAllowed(response, Arrays.asList(HttpMethod.GET));

‎spring-websocket/src/main/java/org/springframework/sockjs/server/SockJsService.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package org.springframework.sockjs.server;
1818

19+
import java.io.IOException;
20+
1921
import org.springframework.http.server.ServerHttpRequest;
2022
import org.springframework.http.server.ServerHttpResponse;
2123
import org.springframework.websocket.HandlerProvider;
@@ -31,6 +33,6 @@ public interface SockJsService {
3133

3234

3335
void handleRequest(ServerHttpRequest request, ServerHttpResponse response, String sockJsPath,
34-
HandlerProvider<WebSocketHandler> handler) throws Exception;
36+
HandlerProvider<WebSocketHandler> handler) throws IOException, TransportErrorException;
3537

3638
}

‎spring-websocket/src/main/java/org/springframework/sockjs/server/TransportErrorException.java

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/*
2+
* Copyright 2002-2013 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.sockjs.server;
18+
19+
import org.springframework.core.NestedRuntimeException;
20+
import org.springframework.websocket.WebSocketHandler;
21+
22+
23+
/**
24+
* Raised when a TransportHandler fails during request processing.
25+
*
26+
* <p>If the underlying exception occurs while sending messages to the client,
27+
* the session will have been closed and the {@link WebSocketHandler} notified.
28+
*
29+
* <p>If the underlying exception occurs while processing an incoming HTTP request
30+
* including posted messages, the session will remain open. Only the incoming
31+
* request is rejected.
32+
*
33+
* @author Rossen Stoyanchev
34+
* @since 4.0
35+
*/
36+
@SuppressWarnings("serial")
37+
public class TransportErrorException extends NestedRuntimeException {
38+
39+
private final String sockJsSessionId;
40+
41+
public TransportErrorException(String msg, Throwable cause, String sockJsSessionId) {
42+
super(msg, cause);
43+
this.sockJsSessionId = sockJsSessionId;
44+
}
45+
46+
public String getSockJsSessionId() {
47+
return sockJsSessionId;
48+
}
49+
50+
@Override
51+
public String getMessage() {
52+
return "Transport error for SockJS session id=" + this.sockJsSessionId + ", " + super.getMessage();
53+
}
54+
55+
}

‎spring-websocket/src/main/java/org/springframework/sockjs/server/TransportHandler.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,6 @@ public interface TransportHandler {
3232
TransportType getTransportType();
3333

3434
void handleRequest(ServerHttpRequest request, ServerHttpResponse response,
35-
HandlerProvider<WebSocketHandler> handler, AbstractSockJsSession session) throws Exception;
35+
HandlerProvider<WebSocketHandler> handler, AbstractSockJsSession session) throws TransportErrorException;
3636

3737
}

‎spring-websocket/src/main/java/org/springframework/sockjs/server/support/DefaultSockJsService.java

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
package org.springframework.sockjs.server.support;
1717

18+
import java.io.IOException;
1819
import java.util.Arrays;
1920
import java.util.Collection;
2021
import java.util.Collections;
@@ -37,6 +38,7 @@
3738
import org.springframework.sockjs.server.AbstractSockJsService;
3839
import org.springframework.sockjs.server.ConfigurableTransportHandler;
3940
import org.springframework.sockjs.server.SockJsService;
41+
import org.springframework.sockjs.server.TransportErrorException;
4042
import org.springframework.sockjs.server.TransportHandler;
4143
import org.springframework.sockjs.server.TransportType;
4244
import org.springframework.sockjs.server.transport.EventSourceTransportHandler;
@@ -140,7 +142,7 @@ public Map<TransportType, TransportHandler> getTransportHandlers() {
140142

141143
@Override
142144
protected void handleRawWebSocketRequest(ServerHttpRequest request, ServerHttpResponse response,
143-
HandlerProvider<WebSocketHandler> handler) throws Exception {
145+
HandlerProvider<WebSocketHandler> handler) throws IOException {
144146

145147
if (isWebSocketEnabled()) {
146148
TransportHandler transportHandler = this.transportHandlers.get(TransportType.WEBSOCKET);
@@ -157,7 +159,8 @@ protected void handleRawWebSocketRequest(ServerHttpRequest request, ServerHttpRe
157159

158160
@Override
159161
protected void handleTransportRequest(ServerHttpRequest request, ServerHttpResponse response,
160-
String sessionId, TransportType transportType, HandlerProvider<WebSocketHandler> handler) throws Exception {
162+
String sessionId, TransportType transportType, HandlerProvider<WebSocketHandler> handler)
163+
throws IOException, TransportErrorException {
161164

162165
TransportHandler transportHandler = this.transportHandlers.get(transportType);
163166

0 commit comments

Comments
 (0)