OutOfMemoryError on GHRepository.readZip due to lack of streaming

[blacelle]

Describe the bug
I encounter OutOfMemoryError over GHRepository.readZip while having reasonable free heap.

To Reproduce
Steps to reproduce the behavior:

1. Consider a large Github repository (e.g. 50MB)
2. Rely on GHRepository.readZip, while being a bit memory constrained (e.g. with Xmx128M)
3. Encounter a OutOfMemoryError

Expected behavior
I would expect the whole InputStream not to be materialized in-memory by org.kohsuke.github.connector.GitHubConnectorResponse.ByteArrayResponse.bodyStream()

Additional context
A typical stack looks like:

Java heap space: java.lang.OutOfMemoryError
java.lang.OutOfMemoryError: Java heap space
	at org.apache.commons.io.output.AbstractByteArrayOutputStream.toByteArrayImpl(AbstractByteArrayOutputStream.java:366)
	at org.apache.commons.io.output.ByteArrayOutputStream.toByteArray(ByteArrayOutputStream.java:163)
	at org.apache.commons.io.IOUtils.toByteArray(IOUtils.java:2241)
	at org.kohsuke.github.connector.GitHubConnectorResponse$ByteArrayResponse.bodyStream(GitHubConnectorResponse.java:187)
	at org.kohsuke.github.Requester.lambda$fetchStream$3(Requester.java:117)
	at org.kohsuke.github.Requester$$Lambda$568/0x00000008404da440.apply(Unknown Source)
	at org.kohsuke.github.GitHubClient.createResponse(GitHubClient.java:485)
	at org.kohsuke.github.GitHubClient.sendRequest(GitHubClient.java:387)
	at org.kohsuke.github.GitHubClient.sendRequest(GitHubClient.java:355)
	at org.kohsuke.github.Requester.fetchStream(Requester.java:117)
	at org.kohsuke.github.GHRepository.downloadArchive(GHRepository.java:3246)
	at org.kohsuke.github.GHRepository.readZip(GHRepository.java:3195)

[bitwiseman]

Agreed, the current implementation has this issue. We chose to do it that way to address other problems in more common scenarios. Allocating some memory that will be quickly returned is okay in most cases.

There are a number of possible fixes, but they add complexity in exchange for memory efficiency. Using file-based caches for example... Or maybe providing a different code path for endpoints likely to return large streams. Something that takes a function letting the client read the http response body stream directly.

PR welcome, but if you decide to do it sketch out what you have in mind before implementing fully so we can agree on the design.

[atsushieno]

I have been using it on my Android app, and this had been a roadblock because it easily goes OutOfMemoryError by platform nature. Fortunately I could simply provide slightly modified version of OkHttpGitHubConnector. It would help if anyone tries to use this library on Android:
https://github.com/atsushieno/android-ci-package-installer/blob/7060e25906c73a507ef6d98cd33bd82cb32229e7/cipackageinstaller/src/main/java/dev/atsushieno/cipackageinstaller/CustomOkHttpGitHubConnector.kt (excuse my Kotlin code here)

[bitwiseman]

@atsushieno
That is interesting, thanks.

Would you be willing to submit a PR with that change?

[atsushieno]

I would usually create PR if it is simply improvements, but I didn't for some reason... what I did ^ is to skip a lot of streaming code in bodySteram() (in Kotlin):

        val wrappedStream by lazy {
            synchronized(this) {
                val rawStream = rawBodyStream()
                wrapStream(rawStream)
            }
        }

        // It is customized point. We do not want huge bytearray
        override fun bodyStream(): InputStream {
            return wrappedStream
        }

It is to override the base implementation in GitHubConnectorResponse.ByteArrayResponse which involves IOUtils.toByteArray() for whatever size of GHArtifact it has:

        public InputStream bodyStream() throws IOException {
            if (isClosed) {
                throw new IOException("Response is closed");
            }
            synchronized (this) {
                if (!inputStreamRead) {
                    InputStream rawStream = rawBodyStream();
                    try (InputStream stream = wrapStream(rawStream)) {
                        if (stream != null) {
                            inputBytes = IOUtils.toByteArray(stream);
                        }
                    }
                    inputStreamRead = true;
                }
            }

            if (inputBytes == null) {
                throw new IOException("Response body missing, stream null");
            }

            return new ByteArrayInputStream(inputBytes);
        }

It goes against what you wrote that you "chose to do it that way to address other problems in more common scenarios". I don't know what those "other problems" actually are, so I left there and did not make it a PR.

[bitwiseman]

Maybe you could add an StreamResponse class, add a method to the request that # switches the response to use it, and make requester.fetchStream() set that switch.

[atsushieno]

I would like to understand the actual reasons that made current implementation. Depending on that, my implementation might be totally missing the points and does not fit general need without further changes, even on Android. I might be just too lucky to have gotten things working.

[bitwiseman]

It's been a while, but the basic reason as I remember it was simplicity and repeatability. From what I remember, there were error scenarios that required reading the same stream more than once.

The reason I asked you to try the above steps is it means you wouldn't need to worry about those scenarios. The streaming reader would only be used in cases where repeat reads don't happen.

[atsushieno]

Ok, but I don't understand what you meant by:

and make requester.fetchStream() set that switch.

Assuming it is about Requester.java, I don't see anything that we can "switch" things. (Generally speaking, I'm not a github-api developer, just an outsider, so I don't understand the internals.)

[bitwiseman]

@atsushieno
Okay, can you submit a PR with the first item and I can wire it in?