Change all response body calls to default to streamed output

Author: bitwiseman

pom.xml

@@ -611,7 +611,7 @@
                 </configuration>
               </execution>
               <execution>
-                <id>httpclient-test</id>
+                <id>httpclient-test-tracing</id>
                 <phase>integration-test</phase>
                 <goals>
                   <goal>test</goal>

src/main/java/org/kohsuke/github/GitHubClient.java

@@ -629,11 +629,8 @@ private void logResponseBody(@Nonnull final GitHubConnectorResponse response) {
         LOGGER.log(FINEST, () -> {
             String body;
             try {
-                if (response.request().avoidBufferedResponseStream()) {
-                    body = "Stream-once body not logged.";
-                } else {
-                    body = GitHubResponse.getBodyAsString(response);
-                }
+                response.forceBufferedBodyStream();
+                body = GitHubResponse.getBodyAsString(response);
             } catch (Throwable e) {
                 body = "Error reading response body";
             }

src/main/java/org/kohsuke/github/GitHubRequest.java

@@ -51,7 +51,6 @@ public class GitHubRequest implements GitHubConnectorRequest {
     private final RateLimitTarget rateLimitTarget;
     private final byte[] body;
     private final boolean forceBody;
-    private final boolean avoidBufferedResponseStream;
 
     private final URL url;
 
@@ -64,8 +63,7 @@ private GitHubRequest(@Nonnull List<Entry> args,
             @Nonnull String method,
             @Nonnull RateLimitTarget rateLimitTarget,
             @CheckForNull byte[] body,
-            boolean forceBody,
-            boolean avoidBufferedResponseStream) {
+            boolean forceBody) {
         this.args = Collections.unmodifiableList(new ArrayList<>(args));
         TreeMap<String, List<String>> caseInsensitiveMap = new TreeMap<>(nullableCaseInsensitiveComparator);
         for (Map.Entry<String, List<String>> entry : headers.entrySet()) {
@@ -79,7 +77,6 @@ private GitHubRequest(@Nonnull List<Entry> args,
         this.rateLimitTarget = rateLimitTarget;
         this.body = body;
         this.forceBody = forceBody;
-        this.avoidBufferedResponseStream = avoidBufferedResponseStream;
         String tailApiUrl = buildTailApiUrl();
         url = getApiURL(apiUrl, tailApiUrl);
     }
@@ -272,14 +269,6 @@ public boolean hasBody() {
         return forceBody || !METHODS_WITHOUT_BODY.contains(method);
     }
 
-    /**
-     * Whether the response to this request should avoid buffered stream or not.
-     */
-    @Override
-    public boolean avoidBufferedResponseStream() {
-        return avoidBufferedResponseStream;
-    }
-
     /**
      * Create a {@link Builder} from this request. Initial values of the builder will be the same as this
      * {@link GitHubRequest}.
@@ -295,8 +284,7 @@ Builder<?> toBuilder() {
                 method,
                 rateLimitTarget,
                 body,
-                forceBody,
-                avoidBufferedResponseStream);
+                forceBody);
     }
 
     private String buildTailApiUrl() {
@@ -366,7 +354,6 @@ static class Builder<B extends Builder<B>> {
 
         private byte[] body;
         private boolean forceBody;
-        private boolean avoidBufferedResponseStream;
 
         /**
          * Create a new {@link GitHubRequest.Builder}
@@ -380,7 +367,6 @@ protected Builder() {
                     "GET",
                     RateLimitTarget.CORE,
                     null,
-                    false,
                     false);
         }
 
@@ -392,8 +378,7 @@ private Builder(@Nonnull List<Entry> args,
                 @Nonnull String method,
                 @Nonnull RateLimitTarget rateLimitTarget,
                 @CheckForNull byte[] body,
-                boolean forceBody,
-                boolean avoidBufferedResponseStream) {
+                boolean forceBody) {
             this.args = new ArrayList<>(args);
             TreeMap<String, List<String>> caseInsensitiveMap = new TreeMap<>(nullableCaseInsensitiveComparator);
             for (Map.Entry<String, List<String>> entry : headers.entrySet()) {
@@ -407,7 +392,6 @@ private Builder(@Nonnull List<Entry> args,
             this.rateLimitTarget = rateLimitTarget;
             this.body = body;
             this.forceBody = forceBody;
-            this.avoidBufferedResponseStream = avoidBufferedResponseStream;
         }
 
         /**
@@ -426,8 +410,7 @@ public GitHubRequest build() {
                     method,
                     rateLimitTarget,
                     body,
-                    forceBody,
-                    avoidBufferedResponseStream);
+                    forceBody);
         }
 
         /**
@@ -817,17 +800,6 @@ public B inBody() {
             forceBody = true;
             return (B) this;
         }
-
-        /**
-         * We cache response stream into buffer by default, but some responses can be huge so we should avoid that.
-         * Setting this flag indicates that we should avoid buffered stream response.
-         *
-         * @return the request builder
-         */
-        public B avoidBufferedResponseStream() {
-            avoidBufferedResponseStream = true;
-            return (B) this;
-        }
     }
 
     /**

src/main/java/org/kohsuke/github/Requester.java

@@ -128,10 +128,7 @@ public int fetchHttpStatusCode() throws IOException {
      *             the io exception
      */
     public <T> T fetchStream(@Nonnull InputStreamFunction<T> handler) throws IOException {
-        return client
-                .sendRequest(this.avoidBufferedResponseStream(),
-                        (connectorResponse) -> handler.apply(connectorResponse.bodyStream()))
-                .body();
+        return client.sendRequest(this, (connectorResponse) -> handler.apply(connectorResponse.bodyStream())).body();
     }
 
     /**

src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java

@@ -81,13 +81,4 @@ public interface GitHubConnectorRequest {
      * @return true, if the body is not null. Otherwise, false.
      */
     boolean hasBody();
-
-    /**
-     * Whether the response stream to this request is not buffered. It is used to avoid huge response caching.
-     *
-     * @return true, if the response stream is not buffered.
-     */
-    default boolean avoidBufferedResponseStream() {
-        return false;
-    }
 }

src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java

@@ -7,7 +7,12 @@
 import java.io.Closeable;
 import java.io.IOException;
 import java.io.InputStream;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Map;
+import java.util.TreeMap;
 import java.util.zip.GZIPInputStream;
 
 import javax.annotation.CheckForNull;
@@ -37,6 +42,12 @@ public abstract class GitHubConnectorResponse implements Closeable {
     private final GitHubConnectorRequest request;
     @Nonnull
     private final Map<String, List<String>> headers;
+    private boolean bodyStreamCalled = false;
+    private boolean bodyBytesLoaded = false;
+    private InputStream bodyStream = null;
+    private byte[] bodyBytes = null;
+    private boolean isClosed = false;
+    private boolean forceBufferedBodyStream;
 
     /**
      * GitHubConnectorResponse constructor
@@ -86,7 +97,55 @@ public String header(String name) {
      *             if response stream is null or an I/O Exception occurs.
      */
     @Nonnull
-    public abstract InputStream bodyStream() throws IOException;
+    public InputStream bodyStream() throws IOException {
+        InputStream body = null;
+        synchronized (this) {
+            if (isClosed) {
+                throw new IOException("Response is closed");
+            }
+
+            if (bodyStreamCalled) {
+                if (!bodyBytesLoaded) {
+                    throw new IOException("Response is already consumed");
+                }
+            } else {
+                body = wrapStream(rawBodyStream());
+                bodyStreamCalled = true;
+                bodyStream = body;
+                if (useBufferedBodyStream()) {
+                    bodyBytesLoaded = true;
+                    try (InputStream stream = body) {
+                        if (stream != null) {
+                            bodyBytes = IOUtils.toByteArray(stream);
+                        }
+                    }
+                    bodyStream = null;
+                }
+            }
+
+            if (bodyBytesLoaded) {
+                body = bodyBytes == null ? null : new ByteArrayInputStream(bodyBytes);
+            }
+        }
+
+        if (body == null) {
+            throw new IOException("Response body missing, stream null");
+        }
+
+        return body;
+    }
+
+    /**
+     * Get the raw implementation specific body stream for this response.
+     *
+     * This method will only be called once to completion. If an exception is thrown, it may be called multiple times.
+     *
+     * @return the stream for the raw response
+     * @throws IOException
+     *             if an I/O Exception occurs.
+     */
+    @CheckForNull
+    protected abstract InputStream rawBodyStream() throws IOException;
 
     /**
      * Gets the {@link GitHubConnector} for this response.
@@ -123,8 +182,33 @@ public Map<String, List<String>> allHeaders() {
      *
      * @return true when unbuffered body stream can should be used.
      */
-    boolean useUnbufferedBodyStream() {
-        return statusCode() == HTTP_OK && request().avoidBufferedResponseStream();
+    boolean useBufferedBodyStream() {
+        synchronized (this) {
+            return forceBufferedBodyStream || statusCode() != HTTP_OK;
+        }
+    }
+
+    /**
+     * Use unbufferred body stream.
+     *
+     * @return true when unbuffered body stream can should be used.
+     */
+    public void forceBufferedBodyStream() {
+        synchronized (this) {
+            this.forceBufferedBodyStream = true;
+        }
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public void close() throws IOException {
+        synchronized (this) {
+            IOUtils.closeQuietly(bodyStream);
+            isClosed = true;
+            this.bodyBytes = null;
+        }
     }
 
     /**
@@ -169,10 +253,6 @@ public final int parseInt(String name) throws NumberFormatException {
      */
     public abstract static class ByteArrayResponse extends GitHubConnectorResponse {
 
-        private boolean inputStreamRead = false;
-        private byte[] inputBytes = null;
-        private boolean isClosed = false;
-
         /**
          * Constructor for ByteArray Response
          *
@@ -188,61 +268,5 @@ protected ByteArrayResponse(@Nonnull GitHubConnectorRequest request,
                 @Nonnull Map<String, List<String>> headers) {
             super(request, statusCode, headers);
         }
-
-        /**
-         * {@inheritDoc}
-         */
-        @Override
-        @Nonnull
-        public InputStream bodyStream() throws IOException {
-            if (isClosed) {
-                throw new IOException("Response is closed");
-            }
-
-            synchronized (this) {
-                InputStream body;
-                if (!inputStreamRead) {
-                    body = wrapStream(rawBodyStream());
-                    if (!useUnbufferedBodyStream()) {
-                        try (InputStream stream = body) {
-                            if (stream != null) {
-                                inputBytes = IOUtils.toByteArray(stream);
-                            }
-                        }
-                    }
-                    inputStreamRead = true;
-                    if (useUnbufferedBodyStream()) {
-                        return body;
-                    }
-                }
-            }
-
-            if (useUnbufferedBodyStream()) {
-                throw new IOException("Response is already consumed");
-            } else if (inputBytes == null) {
-                throw new IOException("Response body missing, stream null");
-            }
-
-            return new ByteArrayInputStream(inputBytes);
-        }
-
-        /**
-         * Get the raw implementation specific body stream for this response.
-         *
-         * This method will only be called once to completion. If an exception is thrown, it may be called multiple
-         * times.
-         *
-         * @return the stream for the raw response
-         * @throws IOException
-         *             if an I/O Exception occurs.
-         */
-        @CheckForNull
-        protected abstract InputStream rawBodyStream() throws IOException;
-
-        @Override
-        public void close() throws IOException {
-            isClosed = true;
-            this.inputBytes = null;
-        }
     }
 }

src/test/java/org/kohsuke/github/GHWorkflowRunTest.java

@@ -19,6 +19,8 @@
 import java.util.Optional;
 import java.util.Scanner;
 import java.util.function.Function;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import java.util.stream.Collectors;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipInputStream;
@@ -394,10 +396,14 @@ public void testArtifacts() throws IOException {
         checkArtifactProperties(artifacts.get(0), "artifact1");
         checkArtifactProperties(artifacts.get(1), "artifact2");
 
+        Logger clientLogger = Logger.getLogger(GitHubClient.class.getName());
+
         // Test download from upload-artifact@v3 infrastructure
         String artifactContent = artifacts.get(0).download((is) -> {
-            assertThat(is, not(isA(ByteArrayInputStream.class)));
-
+            // At finest log level, all body responses are byte arrays.
+            if (clientLogger.getLevel() != Level.FINEST) {
+                assertThat(is, not(isA(ByteArrayInputStream.class)));
+            }
             try (ZipInputStream zis = new ZipInputStream(is)) {
                 StringBuilder sb = new StringBuilder();