From 6faaf3346c70ad9d1f7986a2b432720e765cf0b9 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 3 Oct 2017 15:30:32 -0700 Subject: [PATCH] Issue #272 - Read/Parse exceptions should flow out, so Session can handle it via new Session.close() --- .../jetty/websocket/common/Parser.java | 24 +----- .../jetty/websocket/common/ParserTest.java | 19 +++-- .../common/TextPayloadParserTest.java | 16 ++-- .../websocket/common/ab/TestABCase2.java | 13 ++- .../websocket/common/ab/TestABCase4.java | 79 ++++--------------- .../websocket/common/ab/TestABCase7_3.java | 23 ++---- .../websocket/common/test/UnitParser.java | 4 - 7 files changed, 59 insertions(+), 119 deletions(-) diff --git a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/Parser.java b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/Parser.java index 8cc1d553c25..c0babbd1bb9 100644 --- a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/Parser.java +++ b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/Parser.java @@ -180,7 +180,7 @@ public class Parser return (flagsInUse & 0x10) != 0; } - protected void notifyFrame(final Frame f) + protected void notifyFrame(final Frame f) throws WebSocketException { if (LOG.isDebugEnabled()) LOG.debug("{} Notify {}",policy.getBehavior(),getIncomingFramesHandler()); @@ -221,25 +221,14 @@ public class Parser } catch (WebSocketException e) { - notifyWebSocketException(e); + throw e; } catch (Throwable t) { - LOG.warn(t); - notifyWebSocketException(new WebSocketException(t)); + throw new WebSocketException(t); } } - protected void notifyWebSocketException(WebSocketException e) - { - LOG.warn(e); - if (incomingFramesHandler == null) - { - return; - } - incomingFramesHandler.incomingError(e); - } - public void parse(ByteBuffer buffer) throws WebSocketException { if (buffer.remaining() <= 0) @@ -265,8 +254,6 @@ public class Parser { buffer.position(buffer.limit()); // consume remaining reset(); - // let session know - notifyWebSocketException(e); // need to throw for proper close behavior in connection throw e; } @@ -274,11 +261,8 @@ public class Parser { buffer.position(buffer.limit()); // consume remaining reset(); - // let session know - WebSocketException e = new WebSocketException(t); - notifyWebSocketException(e); // need to throw for proper close behavior in connection - throw e; + throw new WebSocketException(t); } } diff --git a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ParserTest.java b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ParserTest.java index e0bcdeeaca3..92a3bb0ec9a 100644 --- a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ParserTest.java +++ b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ParserTest.java @@ -18,6 +18,7 @@ package org.eclipse.jetty.websocket.common; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; import java.nio.ByteBuffer; @@ -26,6 +27,7 @@ import java.util.Arrays; import java.util.List; import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.websocket.api.ProtocolException; import org.eclipse.jetty.websocket.api.StatusCode; import org.eclipse.jetty.websocket.api.WebSocketBehavior; import org.eclipse.jetty.websocket.api.WebSocketPolicy; @@ -39,10 +41,15 @@ import org.eclipse.jetty.websocket.common.test.UnitGenerator; import org.eclipse.jetty.websocket.common.test.UnitParser; import org.eclipse.jetty.websocket.common.util.Hex; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; public class ParserTest { + @Rule + public ExpectedException expectedException = ExpectedException.none(); + /** * Similar to the server side 5.15 testcase. A normal 2 fragment text text message, followed by another continuation. */ @@ -61,11 +68,9 @@ public class ParserTest IncomingFramesCapture capture = new IncomingFramesCapture(); parser.setIncomingFramesHandler(capture); + expectedException.expect(ProtocolException.class); + expectedException.expectMessage(containsString("CONTINUATION frame without prior !FIN")); parser.parseQuietly(completeBuf); - - capture.assertErrorCount(1); - capture.assertHasFrame(OpCode.TEXT,1); - capture.assertHasFrame(OpCode.CONTINUATION,1); } /** @@ -83,10 +88,10 @@ public class ParserTest UnitParser parser = new UnitParser(); IncomingFramesCapture capture = new IncomingFramesCapture(); parser.setIncomingFramesHandler(capture); - parser.parseQuietly(completeBuf); - capture.assertErrorCount(1); - capture.assertHasFrame(OpCode.TEXT,1); // fragment 1 + expectedException.expect(ProtocolException.class); + expectedException.expectMessage(containsString("Unexpected TEXT frame")); + parser.parseQuietly(completeBuf); } /** diff --git a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/TextPayloadParserTest.java b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/TextPayloadParserTest.java index 19c7982626a..7cbd8fabc72 100644 --- a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/TextPayloadParserTest.java +++ b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/TextPayloadParserTest.java @@ -28,17 +28,21 @@ import java.nio.charset.StandardCharsets; import java.util.Arrays; import org.eclipse.jetty.websocket.api.MessageTooLargeException; -import org.eclipse.jetty.websocket.api.StatusCode; import org.eclipse.jetty.websocket.api.WebSocketBehavior; import org.eclipse.jetty.websocket.api.WebSocketPolicy; import org.eclipse.jetty.websocket.common.test.IncomingFramesCapture; import org.eclipse.jetty.websocket.common.test.UnitParser; import org.eclipse.jetty.websocket.common.util.MaskedByteBuffer; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; public class TextPayloadParserTest { + @Rule + public ExpectedException expectedException = ExpectedException.none(); + @Test public void testFrameTooLargeDueToPolicy() throws Exception { @@ -63,19 +67,15 @@ public class TextPayloadParserTest UnitParser parser = new UnitParser(policy); IncomingFramesCapture capture = new IncomingFramesCapture(); parser.setIncomingFramesHandler(capture); + + expectedException.expect(MessageTooLargeException.class); parser.parseQuietly(buf); - - capture.assertHasErrors(MessageTooLargeException.class,1); - capture.assertHasNoFrames(); - - MessageTooLargeException err = (MessageTooLargeException)capture.getErrors().poll(); - Assert.assertThat("Error.closeCode",err.getStatusCode(),is(StatusCode.MESSAGE_TOO_LARGE)); } @Test public void testLongMaskedText() throws Exception { - StringBuffer sb = new StringBuffer(); ; + StringBuffer sb = new StringBuffer(); for (int i = 0; i < 3500; i++) { sb.append("Hell\uFF4f Big W\uFF4Frld "); diff --git a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase2.java b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase2.java index df12cd6388d..16430c534cc 100644 --- a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase2.java +++ b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase2.java @@ -18,6 +18,7 @@ package org.eclipse.jetty.websocket.common.ab; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; import java.nio.ByteBuffer; @@ -39,11 +40,16 @@ import org.eclipse.jetty.websocket.common.test.IncomingFramesCapture; import org.eclipse.jetty.websocket.common.test.UnitGenerator; import org.eclipse.jetty.websocket.common.test.UnitParser; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; public class TestABCase2 { - WebSocketPolicy policy = new WebSocketPolicy(WebSocketBehavior.CLIENT); + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + private WebSocketPolicy policy = new WebSocketPolicy(WebSocketBehavior.CLIENT); @Test public void testGenerate125OctetPingCase2_4() @@ -315,9 +321,10 @@ public class TestABCase2 UnitParser parser = new UnitParser(policy); IncomingFramesCapture capture = new IncomingFramesCapture(); parser.setIncomingFramesHandler(capture); - parser.parseQuietly(expected); - Assert.assertEquals("error should be returned for too large of ping payload",1,capture.getErrorCount(ProtocolException.class)); + expectedException.expect(ProtocolException.class); + expectedException.expectMessage(containsString("Invalid control frame payload length")); + parser.parseQuietly(expected); } } diff --git a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase4.java b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase4.java index fe7b4ae5eef..e9615b4027b 100644 --- a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase4.java +++ b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase4.java @@ -23,16 +23,19 @@ import java.nio.ByteBuffer; import org.eclipse.jetty.util.log.StacklessLogging; import org.eclipse.jetty.websocket.api.ProtocolException; import org.eclipse.jetty.websocket.api.WebSocketBehavior; -import org.eclipse.jetty.websocket.api.WebSocketException; import org.eclipse.jetty.websocket.api.WebSocketPolicy; import org.eclipse.jetty.websocket.common.Parser; import org.eclipse.jetty.websocket.common.test.IncomingFramesCapture; import org.eclipse.jetty.websocket.common.test.UnitParser; -import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; public class TestABCase4 { + @Rule + public ExpectedException expectedException = ExpectedException.none(); + private WebSocketPolicy policy = new WebSocketPolicy(WebSocketBehavior.CLIENT); @Test @@ -46,25 +49,13 @@ public class TestABCase4 IncomingFramesCapture capture = new IncomingFramesCapture(); - try (StacklessLogging logging = new StacklessLogging(Parser.class)) + try (StacklessLogging ignore = new StacklessLogging(Parser.class)) { Parser parser = new UnitParser(policy); parser.setIncomingFramesHandler(capture); - try - { - parser.parse(expected); - } - catch (ProtocolException ignore) - { - // ignore - } + expectedException.expect(ProtocolException.class); + parser.parse(expected); } - - Assert.assertEquals("error on undefined opcode",1,capture.getErrorCount(WebSocketException.class)); - - Throwable known = capture.getErrors().poll(); - - Assert.assertTrue("undefined option should be in message",known.getMessage().contains("Unknown opcode: 11")); } @Test @@ -78,25 +69,13 @@ public class TestABCase4 IncomingFramesCapture capture = new IncomingFramesCapture(); - try (StacklessLogging logging = new StacklessLogging(Parser.class)) + try (StacklessLogging ignore = new StacklessLogging(Parser.class)) { Parser parser = new UnitParser(policy); parser.setIncomingFramesHandler(capture); - try - { - parser.parse(expected); - } - catch (ProtocolException ignore) - { - // ignore - } + expectedException.expect(ProtocolException.class); + parser.parse(expected); } - - Assert.assertEquals("error on undefined opcode",1,capture.getErrorCount(WebSocketException.class)); - - Throwable known = capture.getErrors().poll(); - - Assert.assertTrue("undefined option should be in message",known.getMessage().contains("Unknown opcode: 12")); } @Test @@ -110,25 +89,13 @@ public class TestABCase4 IncomingFramesCapture capture = new IncomingFramesCapture(); - try (StacklessLogging logging = new StacklessLogging(Parser.class)) + try (StacklessLogging ignore = new StacklessLogging(Parser.class)) { Parser parser = new UnitParser(policy); parser.setIncomingFramesHandler(capture); - try - { - parser.parse(expected); - } - catch (ProtocolException ignore) - { - // ignore - } + expectedException.expect(ProtocolException.class); + parser.parse(expected); } - - Assert.assertEquals("error on undefined opcode",1,capture.getErrorCount(WebSocketException.class)); - - Throwable known = capture.getErrors().poll(); - - Assert.assertTrue("undefined option should be in message",known.getMessage().contains("Unknown opcode: 3")); } @Test @@ -142,24 +109,12 @@ public class TestABCase4 IncomingFramesCapture capture = new IncomingFramesCapture(); - try (StacklessLogging logging = new StacklessLogging(Parser.class)) + try (StacklessLogging ignore = new StacklessLogging(Parser.class)) { Parser parser = new UnitParser(policy); parser.setIncomingFramesHandler(capture); - try - { - parser.parse(expected); - } - catch (ProtocolException ignore) - { - // ignore - } + expectedException.expect(ProtocolException.class); + parser.parse(expected); } - - Assert.assertEquals("error on undefined opcode",1,capture.getErrorCount(WebSocketException.class)); - - Throwable known = capture.getErrors().poll(); - - Assert.assertTrue("undefined option should be in message",known.getMessage().contains("Unknown opcode: 4")); } } diff --git a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase7_3.java b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase7_3.java index 7c523f8bcce..6ecf212f591 100644 --- a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase7_3.java +++ b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/ab/TestABCase7_3.java @@ -18,7 +18,6 @@ package org.eclipse.jetty.websocket.common.ab; -import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; import java.nio.ByteBuffer; @@ -40,11 +39,16 @@ import org.eclipse.jetty.websocket.common.test.UnitGenerator; import org.eclipse.jetty.websocket.common.test.UnitParser; import org.eclipse.jetty.websocket.common.util.Hex; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; public class TestABCase7_3 { - WebSocketPolicy policy = new WebSocketPolicy(WebSocketBehavior.CLIENT); + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + private WebSocketPolicy policy = new WebSocketPolicy(WebSocketBehavior.CLIENT); @Test public void testCase7_3_1GenerateEmptyClose() @@ -83,7 +87,6 @@ public class TestABCase7_3 Frame pActual = capture.getFrames().poll(); Assert.assertThat("CloseFrame.payloadLength",pActual.getPayloadLength(),is(0)); - } @@ -104,13 +107,8 @@ public class TestABCase7_3 UnitParser parser = new UnitParser(policy); IncomingFramesCapture capture = new IncomingFramesCapture(); parser.setIncomingFramesHandler(capture); + expectedException.expect(ProtocolException.class); parser.parseQuietly(expected); - - Assert.assertEquals("error on invalid close payload",1,capture.getErrorCount(ProtocolException.class)); - - ProtocolException known = (ProtocolException)capture.getErrors().poll(); - - Assert.assertThat("Payload.message",known.getMessage(),containsString("Invalid close frame payload length")); } @Test @@ -338,12 +336,7 @@ public class TestABCase7_3 UnitParser parser = new UnitParser(policy); IncomingFramesCapture capture = new IncomingFramesCapture(); parser.setIncomingFramesHandler(capture); + expectedException.expect(ProtocolException.class); parser.parseQuietly(expected); - - Assert.assertEquals("error on invalid close payload",1,capture.getErrorCount(ProtocolException.class)); - - ProtocolException known = (ProtocolException)capture.getErrors().poll(); - - Assert.assertThat("Payload.message",known.getMessage(),containsString("Invalid control frame payload length")); } } diff --git a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/test/UnitParser.java b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/test/UnitParser.java index 11c7e7d0c32..25818ef60a9 100644 --- a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/test/UnitParser.java +++ b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/test/UnitParser.java @@ -58,10 +58,6 @@ public class UnitParser extends Parser { parse(buf); } - catch (Exception ignore) - { - /* ignore */ - } } public void parseSlowly(ByteBuffer buf, int segmentSize)