From f0810c0fa47f66c795966994993d078c9df4761f Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Thu, 23 Dec 2021 16:26:25 +0100 Subject: [PATCH] #7281 review test suite to improve code coverage Signed-off-by: Ludovic Orban --- .../jetty/server/AsyncContentProducer.java | 24 ++- .../server/AsyncContentProducerTest.java | 66 ++++--- .../server/BlockingContentProducerTest.java | 174 +++++++++--------- 3 files changed, 149 insertions(+), 115 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java index 44596e76b6f..959ca0d8438 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java @@ -328,7 +328,7 @@ class AsyncContentProducer implements ContentProducer { if (_transformedContent.isSpecial() || !_transformedContent.isEmpty()) { - if (_transformedContent.isSpecial() && !_error) + if (_transformedContent.getError() != null && !_error) { // In case the _rawContent was set by consumeAll(), check the httpChannel // to see if it has a more precise error. Otherwise, the exact same @@ -436,25 +436,35 @@ class AsyncContentProducer implements ContentProducer HttpInput.Content content = _interceptor.readFrom(_rawContent); if (content != null && content.isSpecial() && !_rawContent.isSpecial()) { - // Set the _error flag to mark the content as definitive, i.e.: - // do not try to produce new raw content to get a fresher error - // when the special content was generated by the interceptor. - _error = true; Throwable error = content.getError(); - if (error != null && _httpChannel.getResponse().isCommitted()) - _httpChannel.abort(error); + if (error != null) + { + // Set the _error flag to mark the content as definitive, i.e.: + // do not try to produce new raw content to get a fresher error + // when the special content was generated by the interceptor. + _error = true; + if (_httpChannel.getResponse().isCommitted()) + _httpChannel.abort(error); + } if (LOG.isDebugEnabled()) LOG.debug("interceptor generated special content {}", this); } else if (content != _rawContent && !_rawContent.isSpecial() && !_rawContent.isEmpty() && _rawContent.remaining() == remainingBeforeInterception) { IOException failure = new IOException("Interceptor " + _interceptor + " did not consume any of the " + _rawContent.remaining() + " remaining byte(s) of content"); + if (content != null) + content.failed(failure); failCurrentContent(failure); // Set the _error flag to mark the content as definitive, i.e.: // do not try to produce new raw content to get a fresher error // when the special content was caused by the interceptor not // consuming the raw content. _error = true; + Response response = _httpChannel.getResponse(); + if (response.isCommitted()) + _httpChannel.abort(failure); + if (LOG.isDebugEnabled()) + LOG.debug("interceptor did not consume content {}", this); content = _transformedContent; } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java index 290da8ecf8a..c260bd3dd03 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java @@ -193,19 +193,20 @@ public class AsyncContentProducerTest public void testAsyncContentProducerInterceptorGeneratesError() { AtomicInteger contentSucceededCount = new AtomicInteger(); - ContentProducer contentProducer = new AsyncContentProducer(new StaticContentHttpChannel(new HttpInput.Content(ByteBuffer.allocate(1)) + ContentProducer contentProducer = new AsyncContentProducer(new ContentListHttpChannel(List.of(new HttpInput.Content(ByteBuffer.allocate(1)) { @Override public void succeeded() { contentSucceededCount.incrementAndGet(); } - })); + }), new HttpInput.EofContent())); try (AutoLock lock = contentProducer.lock()) { contentProducer.setInterceptor(content -> new HttpInput.ErrorContent(new Throwable("testAsyncContentProducerInterceptorGeneratesError interceptor error"))); assertThat(contentProducer.isReady(), is(true)); + assertThat(contentProducer.isError(), is(true)); HttpInput.Content content1 = contentProducer.nextContent(); assertThat(content1.isSpecial(), is(true)); @@ -222,19 +223,20 @@ public class AsyncContentProducerTest public void testAsyncContentProducerInterceptorGeneratesEof() { AtomicInteger contentSucceededCount = new AtomicInteger(); - ContentProducer contentProducer = new AsyncContentProducer(new StaticContentHttpChannel(new HttpInput.Content(ByteBuffer.allocate(1)) + ContentProducer contentProducer = new AsyncContentProducer(new ContentListHttpChannel(List.of(new HttpInput.Content(ByteBuffer.allocate(1)) { @Override public void succeeded() { contentSucceededCount.incrementAndGet(); } - })); + }), new HttpInput.ErrorContent(new Throwable("should not reach this")))); try (AutoLock lock = contentProducer.lock()) { contentProducer.setInterceptor(content -> new HttpInput.EofContent()); assertThat(contentProducer.isReady(), is(true)); + assertThat(contentProducer.isError(), is(false)); HttpInput.Content content1 = contentProducer.nextContent(); assertThat(content1.isSpecial(), is(true)); @@ -251,14 +253,14 @@ public class AsyncContentProducerTest public void testAsyncContentProducerInterceptorThrows() { AtomicInteger contentFailedCount = new AtomicInteger(); - ContentProducer contentProducer = new AsyncContentProducer(new StaticContentHttpChannel(new HttpInput.Content(ByteBuffer.allocate(1)) + ContentProducer contentProducer = new AsyncContentProducer(new ContentListHttpChannel(List.of(new HttpInput.Content(ByteBuffer.allocate(1)) { @Override public void failed(Throwable x) { contentFailedCount.incrementAndGet(); } - })); + }), new HttpInput.EofContent())); try (AutoLock lock = contentProducer.lock()) { contentProducer.setInterceptor(content -> @@ -267,6 +269,7 @@ public class AsyncContentProducerTest }); assertThat(contentProducer.isReady(), is(true)); + assertThat(contentProducer.isError(), is(true)); HttpInput.Content content1 = contentProducer.nextContent(); assertThat(content1.isSpecial(), is(true)); @@ -274,7 +277,7 @@ public class AsyncContentProducerTest HttpInput.Content content2 = contentProducer.nextContent(); assertThat(content2.isSpecial(), is(true)); - assertThat(content1.getError().getCause().getMessage(), is("testAsyncContentProducerInterceptorThrows error")); + assertThat(content2.getError().getCause().getMessage(), is("testAsyncContentProducerInterceptorThrows error")); } assertThat(contentFailedCount.get(), is(1)); } @@ -283,17 +286,25 @@ public class AsyncContentProducerTest public void testAsyncContentProducerInterceptorDoesNotConsume() { AtomicInteger contentFailedCount = new AtomicInteger(); - ContentProducer contentProducer = new AsyncContentProducer(new StaticContentHttpChannel(new HttpInput.Content(ByteBuffer.allocate(1)) + AtomicInteger interceptorContentFailedCount = new AtomicInteger(); + ContentProducer contentProducer = new AsyncContentProducer(new ContentListHttpChannel(List.of(new HttpInput.Content(ByteBuffer.allocate(1)) { @Override public void failed(Throwable x) { contentFailedCount.incrementAndGet(); } - })); + }), new HttpInput.EofContent())); try (AutoLock lock = contentProducer.lock()) { - contentProducer.setInterceptor(content -> null); + contentProducer.setInterceptor(content -> new HttpInput.Content(ByteBuffer.allocate(1)) + { + @Override + public void failed(Throwable x) + { + interceptorContentFailedCount.incrementAndGet(); + } + }); assertThat(contentProducer.isReady(), is(true)); @@ -303,25 +314,26 @@ public class AsyncContentProducerTest HttpInput.Content content2 = contentProducer.nextContent(); assertThat(content2.isSpecial(), is(true)); - assertThat(content1.getError().getMessage(), endsWith("did not consume any of the 1 remaining byte(s) of content")); + assertThat(content2.getError().getMessage(), endsWith("did not consume any of the 1 remaining byte(s) of content")); } assertThat(contentFailedCount.get(), is(1)); + assertThat(interceptorContentFailedCount.get(), is(1)); } @Test - public void testBlockingContentProducerInterceptorDoesNotConsumeEmptyContent() + public void testAsyncContentProducerInterceptorDoesNotConsumeEmptyContent() { AtomicInteger contentSucceededCount = new AtomicInteger(); AtomicInteger specialContentInterceptedCount = new AtomicInteger(); AtomicInteger nullContentInterceptedCount = new AtomicInteger(); - ContentProducer contentProducer = new AsyncContentProducer(new StaticContentHttpChannel(new HttpInput.Content(ByteBuffer.allocate(0)) + ContentProducer contentProducer = new AsyncContentProducer(new ContentListHttpChannel(List.of(new HttpInput.Content(ByteBuffer.allocate(0)) { @Override public void succeeded() { contentSucceededCount.incrementAndGet(); } - })); + }), new HttpInput.EofContent())); try (AutoLock lock = contentProducer.lock()) { contentProducer.setInterceptor(content -> @@ -403,7 +415,7 @@ public class AsyncContentProducerTest } @Test - public void testBlockingContentProducerGzipInterceptorWithError() throws Exception + public void testAsyncContentProducerGzipInterceptorWithError() throws Exception { ByteBuffer[] uncompressedBuffers = new ByteBuffer[3]; uncompressedBuffers[0] = ByteBuffer.wrap("1 hello 1".getBytes(StandardCharsets.ISO_8859_1)); @@ -521,27 +533,33 @@ public class AsyncContentProducerTest } } - private static class StaticContentHttpChannel extends HttpChannel + private static class ContentListHttpChannel extends HttpChannel { - private HttpInput.Content content; + private final List contents; + private final HttpInput.Content finalContent; + private int index; - public StaticContentHttpChannel(HttpInput.Content content) + public ContentListHttpChannel(List contents, HttpInput.Content finalContent) { super(new MockConnector(), new HttpConfiguration(), null, null); - this.content = content; + this.contents = contents; + this.finalContent = finalContent; } @Override public boolean needContent() { - return content != null; + return true; } @Override public HttpInput.Content produceContent() { - HttpInput.Content c = content; - content = new HttpInput.EofContent(); + HttpInput.Content c; + if (index < contents.size()) + c = contents.get(index++); + else + c = finalContent; return c; } @@ -657,12 +675,12 @@ public class AsyncContentProducerTest private static class AccountingInterceptor implements HttpInput.Interceptor { - private List contents = new ArrayList<>(); + private final List contents = new ArrayList<>(); @Override public HttpInput.Content readFrom(HttpInput.Content content) { - if (!contents.contains(content)) + if (content.isSpecial() || !contents.contains(content)) contents.add(content); return content; } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java index 2992b4d7cac..3ffdafa9566 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java @@ -119,90 +119,6 @@ public class BlockingContentProducerTest } } - @Test - public void testBlockingContentProducerInterceptorGeneratesError() - { - AtomicInteger contentSucceededCount = new AtomicInteger(); - ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(new StaticContentHttpChannel(new HttpInput.Content(ByteBuffer.allocate(1)) - { - @Override - public void succeeded() - { - contentSucceededCount.incrementAndGet(); - } - }))); - try (AutoLock lock = contentProducer.lock()) - { - contentProducer.setInterceptor(content -> new HttpInput.ErrorContent(new Throwable("testBlockingContentProducerInterceptorGeneratesError interceptor error"))); - - HttpInput.Content content1 = contentProducer.nextContent(); - assertThat(content1.isSpecial(), is(true)); - assertThat(content1.getError().getMessage(), is("testBlockingContentProducerInterceptorGeneratesError interceptor error")); - - HttpInput.Content content2 = contentProducer.nextContent(); - assertThat(content2.isSpecial(), is(true)); - assertThat(content2.getError().getMessage(), is("testBlockingContentProducerInterceptorGeneratesError interceptor error")); - } - assertThat(contentSucceededCount.get(), is(1)); - } - - @Test - public void testBlockingContentProducerInterceptorGeneratesEof() - { - AtomicInteger contentSucceededCount = new AtomicInteger(); - ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(new StaticContentHttpChannel(new HttpInput.Content(ByteBuffer.allocate(1)) - { - @Override - public void succeeded() - { - contentSucceededCount.incrementAndGet(); - } - }))); - try (AutoLock lock = contentProducer.lock()) - { - contentProducer.setInterceptor(content -> new HttpInput.EofContent()); - - HttpInput.Content content1 = contentProducer.nextContent(); - assertThat(content1.isSpecial(), is(true)); - assertThat(content1.isEof(), is(true)); - - HttpInput.Content content2 = contentProducer.nextContent(); - assertThat(content2.isSpecial(), is(true)); - assertThat(content2.isEof(), is(true)); - } - assertThat(contentSucceededCount.get(), is(1)); - } - - @Test - public void testBlockingContentProducerInterceptorThrows() - { - AtomicInteger contentFailedCount = new AtomicInteger(); - ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(new StaticContentHttpChannel(new HttpInput.Content(ByteBuffer.allocate(1)) - { - @Override - public void failed(Throwable x) - { - contentFailedCount.incrementAndGet(); - } - }))); - try (AutoLock lock = contentProducer.lock()) - { - contentProducer.setInterceptor(content -> - { - throw new RuntimeException("testBlockingContentProducerInterceptorThrows error"); - }); - - HttpInput.Content content1 = contentProducer.nextContent(); - assertThat(content1.isSpecial(), is(true)); - assertThat(content1.getError().getCause().getMessage(), is("testBlockingContentProducerInterceptorThrows error")); - - HttpInput.Content content2 = contentProducer.nextContent(); - assertThat(content2.isSpecial(), is(true)); - assertThat(content2.getError().getCause().getMessage(), is("testBlockingContentProducerInterceptorThrows error")); - } - assertThat(contentFailedCount.get(), is(1)); - } - @Test public void testBlockingContentProducerEofContentIsPassedToInterceptor() throws Exception { @@ -273,6 +189,96 @@ public class BlockingContentProducerTest assertThat(interceptor.contents.get(3).getError().getMessage(), is("testBlockingContentProducerErrorContentIsPassedToInterceptor error")); } + @Test + public void testBlockingContentProducerInterceptorGeneratesError() + { + AtomicInteger contentSucceededCount = new AtomicInteger(); + ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(new StaticContentHttpChannel(new HttpInput.Content(ByteBuffer.allocate(1)) + { + @Override + public void succeeded() + { + contentSucceededCount.incrementAndGet(); + } + }))); + try (AutoLock lock = contentProducer.lock()) + { + contentProducer.setInterceptor(content -> new HttpInput.ErrorContent(new Throwable("testBlockingContentProducerInterceptorGeneratesError interceptor error"))); + + HttpInput.Content content1 = contentProducer.nextContent(); + assertThat(content1.isSpecial(), is(true)); + assertThat(content1.getError().getMessage(), is("testBlockingContentProducerInterceptorGeneratesError interceptor error")); + + assertThat(contentProducer.isError(), is(true)); + + HttpInput.Content content2 = contentProducer.nextContent(); + assertThat(content2.isSpecial(), is(true)); + assertThat(content2.getError().getMessage(), is("testBlockingContentProducerInterceptorGeneratesError interceptor error")); + } + assertThat(contentSucceededCount.get(), is(1)); + } + + @Test + public void testBlockingContentProducerInterceptorGeneratesEof() + { + AtomicInteger contentSucceededCount = new AtomicInteger(); + ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(new StaticContentHttpChannel(new HttpInput.Content(ByteBuffer.allocate(1)) + { + @Override + public void succeeded() + { + contentSucceededCount.incrementAndGet(); + } + }))); + try (AutoLock lock = contentProducer.lock()) + { + contentProducer.setInterceptor(content -> new HttpInput.EofContent()); + + HttpInput.Content content1 = contentProducer.nextContent(); + assertThat(content1.isSpecial(), is(true)); + assertThat(content1.isEof(), is(true)); + + assertThat(contentProducer.isError(), is(false)); + + HttpInput.Content content2 = contentProducer.nextContent(); + assertThat(content2.isSpecial(), is(true)); + assertThat(content2.isEof(), is(true)); + } + assertThat(contentSucceededCount.get(), is(1)); + } + + @Test + public void testBlockingContentProducerInterceptorThrows() + { + AtomicInteger contentFailedCount = new AtomicInteger(); + ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(new StaticContentHttpChannel(new HttpInput.Content(ByteBuffer.allocate(1)) + { + @Override + public void failed(Throwable x) + { + contentFailedCount.incrementAndGet(); + } + }))); + try (AutoLock lock = contentProducer.lock()) + { + contentProducer.setInterceptor(content -> + { + throw new RuntimeException("testBlockingContentProducerInterceptorThrows error"); + }); + + HttpInput.Content content1 = contentProducer.nextContent(); + assertThat(content1.isSpecial(), is(true)); + assertThat(content1.getError().getCause().getMessage(), is("testBlockingContentProducerInterceptorThrows error")); + + assertThat(contentProducer.isError(), is(true)); + + HttpInput.Content content2 = contentProducer.nextContent(); + assertThat(content2.isSpecial(), is(true)); + assertThat(content2.getError().getCause().getMessage(), is("testBlockingContentProducerInterceptorThrows error")); + } + assertThat(contentFailedCount.get(), is(1)); + } + @Test public void testBlockingContentProducerInterceptorDoesNotConsume() {