Skip to content

Commit 959c4e0

Browse files
sbordetsbordet
committed
Fixes COMETD-408 (Allow more detailed and consistent (JS vs Java) error handling).
1 parent 427e356 commit 959c4e0

17 files changed

Lines changed: 328 additions & 130 deletions

File tree

‎cometd-java/cometd-java-client/src/main/java/org/cometd/client/BayeuxClient.java‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,10 @@
1616

1717
package org.cometd.client;
1818

19-
import java.net.ProtocolException;
2019
import java.util.ArrayList;
2120
import java.util.Arrays;
2221
import java.util.Collections;
22+
import java.util.HashMap;
2323
import java.util.Iterator;
2424
import java.util.List;
2525
import java.util.Map;
@@ -47,6 +47,7 @@
4747
import org.cometd.client.transport.TransportRegistry;
4848
import org.cometd.common.AbstractClientSession;
4949
import org.cometd.common.HashMapMessage;
50+
import org.cometd.common.TransportException;
5051
import org.slf4j.Logger;
5152
import org.slf4j.LoggerFactory;
5253

@@ -952,23 +953,29 @@ protected void failMessages(Throwable x, Message... messages)
952953
failed.setId(message.getId());
953954
failed.setSuccessful(false);
954955
failed.setChannel(message.getChannel());
955-
failed.put("message", message);
956-
if (x != null)
957-
failed.put("exception", x);
958956
failed.put(PUBLISH_CALLBACK_KEY, message.remove(PUBLISH_CALLBACK_KEY));
957+
958+
Map<String, Object> failure = new HashMap<String, Object>();
959+
failed.put("failure", failure);
960+
failure.put("message", message);
961+
if (x != null)
962+
failure.put("exception", x);
963+
if (x instanceof TransportException)
964+
failure.putAll(((TransportException)x).getFields());
965+
failure.put(Message.CONNECTION_TYPE_FIELD, getTransport().getName());
966+
959967
receive(failed);
960968
}
961969
}
962970

963971
@Override
964972
protected void notifyListeners(Message.Mutable message)
965973
{
974+
ClientSessionChannel.MessageListener publishCallback = (ClientSessionChannel.MessageListener)message.remove(PUBLISH_CALLBACK_KEY);
966975
if (message.isPublishReply())
967976
{
968977
String messageId = message.getId();
969-
ClientSessionChannel.MessageListener listener = messageId == null ?
970-
(ClientSessionChannel.MessageListener)message.remove(PUBLISH_CALLBACK_KEY) :
971-
publishCallbacks.remove(messageId);
978+
ClientSessionChannel.MessageListener listener = messageId == null ? publishCallback : publishCallbacks.remove(messageId);
972979
if (listener != null)
973980
notifyListener(listener, message);
974981
}
@@ -1156,11 +1163,6 @@ public void onExpire(Message[] messages)
11561163
onFailure(new TimeoutException("expired"), messages);
11571164
}
11581165

1159-
public void onProtocolError(String info, Message[] messages)
1160-
{
1161-
onFailure(new ProtocolException(info), messages);
1162-
}
1163-
11641166
protected void processMessage(Message.Mutable message)
11651167
{
11661168
BayeuxClient.this.processMessage(message);

‎cometd-java/cometd-java-client/src/main/java/org/cometd/client/transport/LongPollingTransport.java‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.text.SimpleDateFormat;
2222
import java.util.ArrayList;
2323
import java.util.Date;
24+
import java.util.HashMap;
2425
import java.util.List;
2526
import java.util.Locale;
2627
import java.util.Map;
@@ -31,6 +32,7 @@
3132

3233
import org.cometd.bayeux.Channel;
3334
import org.cometd.bayeux.Message;
35+
import org.cometd.common.TransportException;
3436
import org.eclipse.jetty.client.ContentExchange;
3537
import org.eclipse.jetty.client.HttpClient;
3638
import org.eclipse.jetty.http.HttpHeaders;
@@ -317,11 +319,20 @@ protected void onResponseComplete() throws IOException
317319
}
318320
}
319321
else
320-
_listener.onProtocolError("Empty response: " + this, _messages);
322+
{
323+
Map<String, Object> failure = new HashMap<String, Object>(2);
324+
// Convert the 200 into 204 (no content)
325+
failure.put("httpCode", 204);
326+
TransportException x = new TransportException(failure);
327+
_listener.onException(x, _messages);
328+
}
321329
}
322330
else
323331
{
324-
_listener.onProtocolError("Unexpected response " + getResponseStatus() + ": " + this, _messages);
332+
Map<String, Object> failure = new HashMap<String, Object>(2);
333+
failure.put("httpCode", getResponseStatus());
334+
TransportException x = new TransportException(failure);
335+
_listener.onException(x, _messages);
325336
}
326337
}
327338

‎cometd-java/cometd-java-client/src/main/java/org/cometd/client/transport/TransportListener.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,4 @@ public interface TransportListener
3131
void onException(Throwable x, Message[] messages);
3232

3333
void onExpire(Message[] messages);
34-
35-
void onProtocolError(String info, Message[] messages);
3634
}

‎cometd-java/cometd-java-client/src/test/java/org/cometd/client/BayeuxClientTest.java‎

Lines changed: 55 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import java.io.EOFException;
2020
import java.io.IOException;
2121
import java.net.ConnectException;
22-
import java.net.ProtocolException;
2322
import java.net.Socket;
2423
import java.net.UnknownHostException;
2524
import java.util.HashMap;
@@ -50,6 +49,7 @@
5049
import org.cometd.client.BayeuxClient.State;
5150
import org.cometd.client.transport.LongPollingTransport;
5251
import org.cometd.common.HashMapMessage;
52+
import org.cometd.common.TransportException;
5353
import org.cometd.server.BayeuxServerImpl;
5454
import org.cometd.server.DefaultSecurityPolicy;
5555
import org.eclipse.jetty.client.ContentExchange;
@@ -203,7 +203,7 @@ protected void customize(ContentExchange exchange)
203203
public void onFailure(Throwable x, Message[] messages)
204204
{
205205
// Suppress logging of expected exception
206-
if (!(x instanceof ProtocolException))
206+
if (!(x instanceof TransportException))
207207
super.onFailure(x, messages);
208208
}
209209
};
@@ -369,17 +369,17 @@ public void onMessage(ClientSessionChannel session, Message message)
369369
Message message = queue.poll(backoffIncrement, TimeUnit.MILLISECONDS);
370370
Assert.assertFalse(message.isSuccessful());
371371
Object exception = message.get("exception");
372-
Assert.assertTrue(exception instanceof ProtocolException);
372+
Assert.assertTrue(exception instanceof TransportException);
373373

374374
message = queue.poll(2000 + 2 * backoffIncrement, TimeUnit.MILLISECONDS);
375375
Assert.assertFalse(message.isSuccessful());
376376
exception = message.get("exception");
377-
Assert.assertTrue(exception instanceof ProtocolException);
377+
Assert.assertTrue(exception instanceof TransportException);
378378

379379
message = queue.poll(2000 + 3 * backoffIncrement, TimeUnit.MILLISECONDS);
380380
Assert.assertFalse(message.isSuccessful());
381381
exception = message.get("exception");
382-
Assert.assertTrue(exception instanceof ProtocolException);
382+
Assert.assertTrue(exception instanceof TransportException);
383383

384384
filter.code = 0;
385385

@@ -658,11 +658,14 @@ public void onMessage(ClientSessionChannel channel, Message message)
658658

659659
// If port 80 is listening, it's probably some other HTTP server
660660
// and a bayeux request will result in a 404, which is converted
661-
// to a ProtocolException; if not listening, it will be a ConnectException
661+
// to a TransportException; if not listening, it will be a ConnectException
662+
Map<String, Object> failure = (Map<String, Object>)message.get("failure");
663+
Assert.assertNotNull(failure);
664+
Object exception = failure.get("exception");
662665
if (listening.get())
663-
Assert.assertTrue(message.get("exception") instanceof ProtocolException);
666+
Assert.assertTrue(exception instanceof TransportException);
664667
else
665-
Assert.assertTrue(message.get("exception") instanceof ConnectException);
668+
Assert.assertTrue(exception instanceof ConnectException);
666669
latch.countDown();
667670
}
668671
});
@@ -1225,6 +1228,50 @@ public void onMessage(ClientSessionChannel channel, Message message)
12251228
disconnectBayeuxClient(client);
12261229
}
12271230

1231+
@Test
1232+
public void testHandshakeOverHTTPReportsHTTPFailure() throws Exception
1233+
{
1234+
startServer(null);
1235+
// No transports on server, to make the client fail
1236+
((BayeuxServerImpl)bayeux).setAllowedTransports();
1237+
1238+
BayeuxClient client = new BayeuxClient(cometdURL, new LongPollingTransport(null, httpClient))
1239+
{
1240+
@Override
1241+
public void onFailure(Throwable x, Message[] messages)
1242+
{
1243+
// Suppress logging of expected exception
1244+
if (!(x instanceof TransportException))
1245+
super.onFailure(x, messages);
1246+
}
1247+
};
1248+
client.setDebugEnabled(debugTests());
1249+
final CountDownLatch latch = new CountDownLatch(1);
1250+
client.getChannel(Channel.META_HANDSHAKE).addListener(new ClientSessionChannel.MessageListener()
1251+
{
1252+
public void onMessage(ClientSessionChannel channel, Message message)
1253+
{
1254+
// Verify the failure object is there
1255+
Map<String, Object> failure = (Map<String, Object>)message.get("failure");
1256+
Assert.assertNotNull(failure);
1257+
// Verify that the transport is there
1258+
Assert.assertEquals("long-polling", failure.get(Message.CONNECTION_TYPE_FIELD));
1259+
// Verify the original message is there
1260+
Assert.assertNotNull(failure.get("message"));
1261+
// Verify the HTTP status code is there
1262+
Assert.assertEquals(400, failure.get("httpCode"));
1263+
// Verify the exception string is there
1264+
Assert.assertNotNull(failure.get("exception"));
1265+
latch.countDown();
1266+
}
1267+
});
1268+
client.handshake();
1269+
1270+
Assert.assertTrue(latch.await(5, TimeUnit.SECONDS));
1271+
1272+
disconnectBayeuxClient(client);
1273+
}
1274+
12281275
private class DumpThread extends Thread
12291276
{
12301277
public void run()

‎cometd-java/cometd-java-client/src/test/java/org/cometd/client/transport/LongPollingTransportTest.java‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.cometd.bayeux.Message;
2828
import org.cometd.bayeux.Message.Mutable;
2929
import org.cometd.common.HashMapMessage;
30+
import org.cometd.common.TransportException;
3031
import org.eclipse.jetty.client.HttpClient;
3132
import org.junit.Rule;
3233
import org.junit.Test;
@@ -196,9 +197,10 @@ public void run()
196197
transport.send(new EmptyTransportListener()
197198
{
198199
@Override
199-
public void onProtocolError(String info, Message[] messages)
200+
public void onException(Throwable x, Message[] messages)
200201
{
201-
latch.countDown();
202+
if (x instanceof TransportException)
203+
latch.countDown();
202204
}
203205
});
204206
long end = System.nanoTime();
@@ -418,9 +420,5 @@ public void onException(Throwable x, Message[] messages)
418420
public void onExpire(Message[] messages)
419421
{
420422
}
421-
422-
public void onProtocolError(String info, Message[] messages)
423-
{
424-
}
425423
}
426424
}

‎cometd-java/cometd-java-common/src/main/java/org/cometd/common/TransportException.java‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
/*
2+
* Copyright (c) 2010 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.cometd.common;
18+
19+
import java.util.Map;
20+
21+
public class TransportException extends RuntimeException
22+
{
23+
private final Map<String, Object> fields;
24+
25+
public TransportException(Map<String, Object> fields)
26+
{
27+
this.fields = fields;
28+
}
29+
30+
public TransportException(Throwable cause, Map<String, Object> fields)
31+
{
32+
super(cause);
33+
this.fields = fields;
34+
}
35+
36+
public Map<String, Object> getFields()
37+
{
38+
return fields;
39+
}
40+
}

‎cometd-java/cometd-websocket-jetty/src/main/java/org/cometd/websocket/client/WebSocketTransport.java‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import java.net.ProtocolException;
2323
import java.net.SocketTimeoutException;
2424
import java.net.URI;
25-
import java.net.URISyntaxException;
2625
import java.text.ParseException;
2726
import java.util.ArrayList;
2827
import java.util.Collections;
@@ -35,13 +34,16 @@
3534
import java.util.concurrent.ScheduledFuture;
3635
import java.util.concurrent.TimeUnit;
3736
import java.util.concurrent.TimeoutException;
37+
import java.util.regex.Matcher;
38+
import java.util.regex.Pattern;
3839

3940
import org.cometd.bayeux.Channel;
4041
import org.cometd.bayeux.Message;
4142
import org.cometd.bayeux.Message.Mutable;
4243
import org.cometd.client.transport.HttpClientTransport;
4344
import org.cometd.client.transport.MessageClientTransport;
4445
import org.cometd.client.transport.TransportListener;
46+
import org.cometd.common.TransportException;
4547
import org.eclipse.jetty.websocket.WebSocket;
4648
import org.eclipse.jetty.websocket.WebSocket.Connection;
4749
import org.eclipse.jetty.websocket.WebSocketClient;
@@ -237,11 +239,7 @@ private Connection connect(TransportListener listener, Mutable[] messages)
237239
_connection = client.open(uri, _websocket, getConnectTimeout(), TimeUnit.MILLISECONDS);
238240

239241
if (_aborted)
240-
{
241242
listener.onException(new IOException("Aborted"), messages);
242-
}
243-
244-
return _connection;
245243
}
246244
catch (ConnectException x)
247245
{
@@ -255,22 +253,29 @@ private Connection connect(TransportListener listener, Mutable[] messages)
255253
{
256254
listener.onConnectException(x, messages);
257255
}
258-
catch (URISyntaxException x)
259-
{
260-
_webSocketSupported = false;
261-
listener.onProtocolError(x.getMessage(), messages);
262-
}
263256
catch (InterruptedException x)
264257
{
265-
_webSocketSupported = false;
266-
listener.onException(x, messages);
258+
listener.onConnectException(x, messages);
267259
}
268260
catch (ProtocolException x)
269261
{
262+
// Probably a WebSocket upgrade failure
270263
_webSocketSupported = false;
271-
listener.onProtocolError(x.getMessage(), messages);
264+
// Try to parse the HTTP error, although it's ugly
265+
Map<String, Object> failure = new HashMap<String, Object>(2);
266+
failure.put("websocketCode", 1002);
267+
// Unfortunately the information on the HTTP status code is not available directly
268+
// Try to parse it although it's ugly
269+
Matcher matcher = Pattern.compile("(\\d+){3}").matcher(x.getMessage());
270+
if (matcher.find())
271+
{
272+
int code = Integer.parseInt(matcher.group());
273+
if (code > 100 && code < 600)
274+
failure.put("httpCode", code);
275+
}
276+
listener.onException(new TransportException(x, failure), messages);
272277
}
273-
catch (IOException x)
278+
catch (Exception x)
274279
{
275280
_webSocketSupported = false;
276281
listener.onException(x, messages);

0 commit comments

Comments
 (0)