Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fіx possible OOM in downloadFileAsStreamAsync #1467

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

aNNiMON
Copy link
Contributor

@aNNiMON aNNiMON commented Dec 26, 2024

I've been using the -Xmx80M JVM option since telegrambots 5.x. Recently I got an OOM error on telegrambots 8.0.0 after trying to download a 17.8M file:

Exception in thread "OkHttp Dispatcher" java.lang.OutOfMemoryError: Java heap space
    at org.apache.commons.io.IOUtils.byteArray(IOUtils.java:368)
    at org.apache.commons.io.output.AbstractByteArrayOutputStream.toByteArrayImpl(AbstractByteArrayOutputStream.java:201)                                                             
    at org.apache.commons.io.output.UnsynchronizedByteArrayOutputStream.toByteArray(UnsynchronizedByteArrayOutputStream.java:184)                                                     
    at org.apache.commons.io.IOUtils.toByteArray(IOUtils.java:2703)                      
    at org.telegram.telegrambots.client.okhttp.OkHttpFutureDownloadCallback.onResponse(OkHttpFutureDownloadCallback.java:28)                                                          

This PR fixes the issue

Copy link
Owner

@rubenlagus rubenlagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test inside TestTelegramClientIntegration to ensure it works as expected

@aNNiMON
Copy link
Contributor Author

aNNiMON commented Dec 31, 2024

@rubenlagus tests have been added.

Now I see there are two possible usages of downloadFileAsStreamAsync:

  1. As a callback chain: client.downloadFileAsStreamAsync(..).thenAccept(is -> /* read file */).then(...)
  2. Get InputStream from completableFuture: InputStream is = client.downloadFileAsStreamAsync(...).get().

The problem with the second usage is that we cannot get the InputStream out of the completableFuture without closing the InputStream/responseBody in the OkHttpFutureDownloadCallback.

By making the InputStream buffered, we can safely return a stream from completableFuture:

InputStream is = client.downloadFileAsStreamAsync(...)..thenApply(is -> {
    try {
        return IOUtils.toBufferedInputStream(is);
    } catch (IOException e) {
        throw new RuntimeException(e);
    }
}).get();

I've done this for InputStream downloadFileAsStream(File file), but users should do the same if they use client.downloadFileAsStreamAsync(...).get().


I also though replacing complete(body.byteStream()); with complete(IOUtils.toBufferedInputStream(body.byteStream()));, but it still consumes additional memory.

Here are my observations for downloading 18_439_895 bytes:

  • complete(new ByteArrayInputStream(IOUtils.toByteArray(body.byteStream()))); -> OOM on -Xmx80M:
  • complete(IOUtils.toBufferedInputStream(body.byteStream())); -> Works on -Xmx80M, OOM on -Xmx40M:
  • complete(body.byteStream()); -> works -Xmx80M as well as -Xmx40M

@aNNiMON aNNiMON changed the title Fox possible OOM in downloadFileAsStreamAsync Fіx possible OOM in downloadFileAsStreamAsync Jan 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants