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

Fix copy large files #8409

Merged
merged 4 commits into from
Jul 4, 2024
Merged

Fix copy large files #8409

merged 4 commits into from
Jul 4, 2024

Conversation

joschi
Copy link
Contributor

@joschi joschi commented Feb 29, 2024

This is an attempt to fix OutOfMemoryError (out of heap memory) when copying large files into a container via GenericContainer#withCopyFileToContainer().

The previous implementation relied on an in-memory array backing a ByteArrayInputStream which represents the tar archive sent to the Docker daemon which is limited by the available heap memory of the JVM at the moment of execution.

By using the local disk (temporary directory), we can copy files into the container which are larger than the available heap memory of the JVM. The caveat being that we are now limited by the available disk space in the temporary directory of the machine running Testcontainers.

Maybe it could be useful to perform this conditionally depending on the size of the transferables or configurable via runtime property.

What do you think?

Fixes #4203

@joschi joschi requested a review from a team as a code owner February 29, 2024 21:31
@joschi joschi force-pushed the issue-4203 branch 2 times, most recently from 16693a6 to 3051f1f Compare March 5, 2024 21:29
@eddumelendez
Copy link
Member

Hi @joschi, thanks for the PR. What I had in mind for the issue is checking if copy file is supported to do an async operation and then implement it in docker-java and use it in Testcontainers.

This is an attempt to fix `OutOfMemoryError` (out of heap memory) when copying large files into a container via `GenericContainer#withCopyFileToContainer()`.

The previous implementation relied on an in-memory array backing a `ByteArrayInputStream` which represents the tar archive sent to the Docker daemon which is limited by the available heap memory of the JVM at the moment of execution.

By using the local disk (temporary directory), we can copy files into the container which are larger than the available heap memory of the JVM.
The caveat being that we are now limited by the available disk space in the temporary directory of the machine running Testcontainers.

Fixes testcontainers#4203
@eddumelendez eddumelendez added this to the next milestone Jul 4, 2024
@eddumelendez eddumelendez changed the title Use local disk for GenericContainer#withCopyFileToContainer() Fix copy large files Jul 4, 2024
@eddumelendez eddumelendez merged commit b4b1c20 into testcontainers:main Jul 4, 2024
99 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution, @joschi ! PR has been updated to stream the files instead of use local disk.

@frankjkelly
Copy link

Fixing #7239 is amazing. Thank you so much!
Is there any thought or plan at this stage as to when we might get a release with the fix?
Thanks to all involved!

lizhou1111 pushed a commit to lizhou1111/testcontainers-java that referenced this pull request Jul 13, 2024
Fix `OutOfMemoryError` (out of heap memory) when copying large files into a container via `GenericContainer#withCopyFileToContainer()`.

Fixes testcontainers#4203

---------

Co-authored-by: Eddú Meléndez <eddu.melendez@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GenericContainer#withCopyFileToContainer is not efficient memory-wise
3 participants