8000 Fix memory leak caused by S3 DownloadFileObj by viren-nadkarni · Pull Request #11674 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

Fix memory leak caused by S3 DownloadFileObj #11674

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 11, 2024
Merged

Fix memory leak caused by S3 DownloadFileObj #11674

merged 1 commit into from
Oct 11, 2024
+7 −9

Conversation

viren-nadkarni
Copy link
Member
@viren-nadkarni viren-nadkarni commented Oct 11, 2024 •
edited
Loading

Background

We started experiencing BigData test suite OOM failures in the CI since 9th October 3PM CET. Investigations revealed that there was a memory leak in boto3 DownloadFileObj and DownloadFile but only when running in the same pytest process as LocalStack.

There are some untested hypothesis around why this is happening. These operations are implemented in S3 Transfer library (which can fall back to awscrt). This also uses a thread pool executor, is Pytest interfering somehow?

Changes

For now, the workaround is to use S3 GetObject. There is a slight performance impact, but since the affected code is a testutil and used in the test suite. There will be no impact on LocalStack.

Related

Possibly related to:

@viren-nadkarni viren-nadkarni self-assigned this Oct 11, 2024
@viren-nadkarni viren-nadkarni added the semver: patch Non-breaking changes which can be included in patch releases label Oct 11, 2024
bentsku
bentsku approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor
@bentsku bentsku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Thanks for investigating it, really weird issue, hopefully we can find the root cause, but this is a good workaround for now. Glad the issue doesn't affect LocalStack itself but just our pipeline 🙏

@viren-nadkarni viren-nadkarni marked this pull request as ready for review October 11, 2024 10:55
@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 40m 37s ⏱️ - 1m 24s
3 492 tests ±0  3 078 ✅ ±0  414 💤 ±0  0 ❌ ±0 
3 494 runs  ±0  3 078 ✅ ±0  416 💤 ±0  0 ❌ ±0 

Results for commit c8b6646. ± Comparison against base commit 3026d5c.

@viren-nadkarni viren-nadkarni merged commit 951e41a into master Oct 11, 2024
40 of 41 checks passed
@viren-nadkarni viren-nadkarni deleted the mem-leak-boto branch October 11, 2024 11:36
macnev2013 pushed a commit that referenced this pull request Oct 22, 2024
@viren-nadkarni @macnev2013
Fix memory leak caused by S3 DownloadFileObj (#11674)
3694874
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

@macnev2013 macnev2013 Awaiting requested review from macnev2013

Assignees

@viren-nadkarni viren-nadkarni

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@viren-nadkarni @bentsku
0