Skip to content

fix(storage): Prevent duplicate observer registration in TransferWorkerObserver to mitigate OOM issues #2988

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

Merged
merged 6 commits into from
Feb 24, 2025

Conversation

onseok
Copy link
Contributor

@onseok onseok commented Feb 14, 2025

  • PR title and description conform to Pull Request guidelines.

Issue #, if available:
Issue #2840

Description of changes:
The error appears on devices with limited heap (for example, some tablets or set-top boxes running Android 7.1.2/OS 14) when many pending transfers trigger multiple LiveData observer registrations in the TransferWorkerObserver. These duplicate observer registrations may lead to excessive internal allocations (via Room’s InvalidationTracker, MapBuilder, and SetBuilder), eventually causing OOM.

To address this, the PR introduces a thread-safe set (observedTags) that tracks which transfer tags have already been observed. In the attachObserver() method, we now check if the tag is already present in observedTags and return early if so; otherwise, the observer is registered and the tag is added to the set. Similarly, in removeObserver(), we first verify that the tag exists in observedTags before unregistering the observer and then remove the tag. This change prevents the same transfer from being observed multiple times, reducing memory pressure and mitigating OOM issues without impacting the core functionality of transfer monitoring.

How did you test these changes?
Due to the internal and private visibility of several classes and methods in this module, direct testing of these functions is challenging.

Documentation update required?

  • No
  • Yes (Please include a PR link for the documentation update)

General Checklist

  • Added Unit Tests
  • Added Integration Tests
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@onseok onseok requested a review from a team as a code owner February 14, 2025 18:58
@onseok onseok force-pushed the fix/transfer-worker-observer-oom branch from f599641 to 9b11da8 Compare February 14, 2025 19:02
@onseok onseok changed the title fix(storage): Prevent duplicate observer registration in TransferWorkerObserver to mitigate OOM issues (#2840) fix(storage): Prevent duplicate observer registration in TransferWorkerObserver to mitigate OOM issues Feb 14, 2025
@tylerjroach
Copy link
Member

Thank you for the PR, someone on our team will take a look and get back to you if we have any questions!

Copy link
Member

@mattcreaser mattcreaser left a comment

Choose a reason for hiding this comment

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

Hi @onseok. The idea of the change looks good to me but you missed adding the tag to the set in attachObserver. We can also take advantage of the return values of add/remove.

@mattcreaser
Copy link
Member

Once the above updates are made we can run tests and get this merged. Thanks for the contribution! 🚀

@onseok
Copy link
Contributor Author

onseok commented Feb 21, 2025

Hi @mattcreaser, thank you for the feedback! I've updated the PR to use the return values of add() and remove() for the observedTags set to control observer registration and removal. Please let me know if there are any further suggestions.

@onseok onseok requested a review from mattcreaser February 21, 2025 20:53
mattcreaser
mattcreaser previously approved these changes Feb 22, 2025
Copy link
Member

@mattcreaser mattcreaser left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've started running tests and once we get passes we can merge this to include in our next release.

@mattcreaser
Copy link
Member

mattcreaser commented Feb 22, 2025

Hi @onseok the PR is failing lint check. Please run this command and ensure it passes: ./gradlew :aws-storage-s3:ktlintCheck

@onseok
Copy link
Contributor Author

onseok commented Feb 22, 2025

Hi @mattcreaser, I’ve fixed it by reordering the imports. It should pass ./gradlew :aws-storage-s3:ktlintCheck now!

@mattcreaser mattcreaser merged commit 416c971 into aws-amplify:main Feb 24, 2025
3 checks passed
@mattcreaser
Copy link
Member

@onseok this PR has been merged and will be included in our next release. Thanks again!

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.

3 participants