[solr] branch jira/solr-16129 updated: SOLR-16129: tests w/nocommits showing why the test passes for the wrong reason and how to make it fail for the right reason

[ho...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

hossman pushed a commit to branch jira/solr-16129
in repository https://gitbox.apache.org/repos/asf/solr.git


The following commit(s) were added to refs/heads/jira/solr-16129 by this push:
     new ac602a8  SOLR-16129: tests w/nocommits showing why the test passes for the wrong reason and how to make it fail for the right reason
ac602a8 is described below

commit ac602a8438b04aa527232c5652a819ff1d407b77
Author: Chris Hostetter <ho...@apache.org>
AuthorDate: Thu Mar 31 19:11:54 2022 -0700

    SOLR-16129: tests w/nocommits showing why the test passes for the wrong reason and how to make it fail for the right reason
---
 .../solrj/util/InputStreamResponseListener.java    |   9 +-
 .../util/TestInputStreamResponseListener.java      | 112 +++++++++++++++++++++
 2 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/util/InputStreamResponseListener.java b/solr/solrj/src/java/org/apache/solr/client/solrj/util/InputStreamResponseListener.java
index 688566b..ef7a921 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/util/InputStreamResponseListener.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/util/InputStreamResponseListener.java
@@ -48,6 +48,10 @@ import org.eclipse.jetty.util.IO;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 
+// nocommit: we need to switch from using 'Object lock' to using a ReentrantLock w/Condition we can await on
+// nocommit: safest plan is to adopt all changes in https://github.com/eclipse/jetty.project/pull/7260 as is ...
+// nocommit: ...(while also pulling in "AutoLock") and add requestTimeout logic to await() call
+
 /**
  * Fork of jetty's <code>InputStreamResponseListener</code> that adds support for an (optional)
  * <code>requestTimeout</code> (which defaults to 1 hour from instantiation) as well as a
@@ -86,7 +90,7 @@ public class InputStreamResponseListener extends Listener.Adapter {
   private static final Logger log = Log.getLogger(InputStreamResponseListener.class);
   private static final DeferredContentProvider.Chunk EOF =
       new DeferredContentProvider.Chunk(BufferUtil.EMPTY_BUFFER, Callback.NOOP);
-  private final Object lock = this;
+  private final Object lock = this; 
   private final CountDownLatch responseLatch = new CountDownLatch(1);
   private final CountDownLatch resultLatch = new CountDownLatch(1);
   private final AtomicReference<InputStream> stream = new AtomicReference<>();
@@ -316,12 +320,15 @@ public class InputStreamResponseListener extends Listener.Adapter {
 
             if (closed) throw new AsynchronousCloseException();
 
+            // nocommit: this check shouldn't be needed/used here - the await call should tell us to Timeout
             if (requestTimeout.isBefore(Instant.now()))
               throw new TimeoutException("requestTimeout exceeded");
 
             // NOTE: convert maxWaitLimit to Instant for comparison, rather then vice-versa, so we
             // don't risk ArithemticException
             final Instant now = Instant.now();
+            // nocommit: replace this with await, if result is false throw TimeoutException...
+            // nocommit: ...exception message should mention waitLimit, unless requestTime.isBefore(now())
             lock.wait(
                 now.plusMillis(maxWaitLimit).isBefore(requestTimeout)
                     ? maxWaitLimit
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/util/TestInputStreamResponseListener.java b/solr/solrj/src/test/org/apache/solr/client/solrj/util/TestInputStreamResponseListener.java
new file mode 100644
index 0000000..742fbce
--- /dev/null
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/util/TestInputStreamResponseListener.java
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.solrj.util;
+
+import java.io.InputStream;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.Collections;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ForkJoinPool;
+import java.util.concurrent.ForkJoinTask;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.client.solrj.impl.HttpClientUtil;
+
+import org.eclipse.jetty.client.HttpResponse;
+import org.eclipse.jetty.util.Callback;
+
+import static org.hamcrest.core.StringContains.containsString;
+
+public class TestInputStreamResponseListener extends SolrTestCase {
+
+  public void testNoDataTriggersWaitLimit() throws Exception {
+    final long waitLimit = 1000; // millis
+    final InputStreamResponseListener listener = new InputStreamResponseListener(waitLimit);
+
+    // nocommit: we should be able to use a null requestTimeout to pass this test....
+    // nocommit: (the waitLimit should be enough to trigger failure)
+    listener.setRequestTimeout(Instant.now()); // nocommit
+    // nocommit: // listener.setRequestTimeout(null);
+    
+    // emulate low level transport code providing headers, and then nothing else...
+    final HttpResponse dummyResponse = new HttpResponse(null /* bogus request */, Collections.emptyList());
+    listener.onHeaders(dummyResponse);
+
+    // client tries to consume, but there is never any content...
+    assertEquals(dummyResponse, listener.get(0, TimeUnit.SECONDS));
+    final ForkJoinTask<IOException> readTask = ForkJoinPool.commonPool().submit(() -> {
+        try (final InputStream stream = listener.getInputStream()) {
+          return expectThrows(IOException.class, () -> {
+              int trash = stream.read();
+            });
+        }
+      });
+    final IOException expected = readTask.get(waitLimit * 2L, TimeUnit.MILLISECONDS);
+    assertNotNull(expected.getCause());
+    assertEquals(TimeoutException.class, expected.getCause().getClass());
+
+    // nocommit: this should be something about waitLimit...
+    assertThat(expected.getCause().getMessage(), containsString("requestTimeout exceeded"));
+  }
+
+
+      
+  public void testReallySlowDataTriggersRequestTimeout() throws Exception {
+    final long writeDelayMillies = 500;
+    final InputStreamResponseListener listener = new InputStreamResponseListener(writeDelayMillies * 2);
+    
+    // emulate low level transport code providing headers, and then writes a (slow) never ending stream of bytes
+    final HttpResponse dummyResponse = new HttpResponse(null /* bogus request */, Collections.emptyList());
+    listener.onHeaders(dummyResponse);
+    final CountDownLatch writeTaskCloseLatch = new CountDownLatch(1);
+    try {
+      final ForkJoinTask<Boolean> writeTask = ForkJoinPool.commonPool().submit(() -> {
+          final ByteBuffer dataToWriteForever = ByteBuffer.allocate(5);
+          while (0 < writeTaskCloseLatch.getCount()) {
+            dataToWriteForever.position(0);
+            listener.onContent(dummyResponse, dataToWriteForever, Callback.NOOP);
+            Thread.sleep(writeDelayMillies);
+          }
+          return true;
+        });
+
+      // client reads "forever" ... until read times out because requestTimeout exceeded
+      assertEquals(dummyResponse, listener.get(0, TimeUnit.SECONDS));
+      final IOException expected = expectThrows(IOException.class, () -> {
+          final Instant requestTimeout = Instant.now().plus(1, ChronoUnit.MINUTES);
+          listener.setRequestTimeout(requestTimeout);
+          final Instant forever = requestTimeout.plusSeconds(60);
+          try (final InputStream stream = listener.getInputStream()) {
+            while (Instant.now().isBefore(forever)) {
+              int trash = stream.read(); // this should eventually throw an exception
+            }
+          }
+        });
+      assertNotNull(expected.getCause());
+      assertEquals(TimeoutException.class, expected.getCause().getClass());
+      assertThat(expected.getCause().getMessage(), containsString("requestTimeout exceeded"));
+    } finally {
+      writeTaskCloseLatch.countDown();
+    }
+  }
+}