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: Prevent a memory leak on AndroidView which may not call ON_DESTROY #188

Merged
merged 1 commit into from Sep 21, 2022

Conversation

polivmi1
Copy link
Contributor

@polivmi1 polivmi1 commented Aug 14, 2022

This PR fixes a memory leak that can happen in AndroidView when navigating to another screen.
The Lifecycle ON_DESTROY event is never called because in onDispose we remove the mapLifecycleObserver and this method is called before the Lifecycle ON_DESTROY event.

We have to handle any Lifecycle ON_DESTROY in the onDispose method.


Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open a GitHub issue as a bug/feature request before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #138 🦕

@google-cla
Copy link

google-cla bot commented Aug 14, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@polivmi1
Copy link
Contributor Author

I signed it.

@wangela wangela assigned wangela and unassigned barbeau Aug 18, 2022
@arriolac arriolac changed the title #138 - prevent a memory leak on AndroidView which may not call ON_DES… fix: Prevent a memory leak on AndroidView which may not call ON_DESTROY Aug 24, 2022
@polivmi1
Copy link
Contributor Author

@arriolac I see the test problems have been fixed - can we re-run this and merge unless there is no problem?

@arriolac
Copy link
Member

This should be good to merge. Will let @wangela take over from here

@wangela wangela merged commit b87f837 into googlemaps:main Sep 21, 2022
@wangela
Copy link
Member

wangela commented Sep 21, 2022

Thank you for your contribution @polivmi1 !

wangela added a commit that referenced this pull request Sep 21, 2022
wangela added a commit that referenced this pull request Sep 21, 2022
wangela added a commit that referenced this pull request Sep 21, 2022
googlemaps-bot pushed a commit that referenced this pull request Sep 21, 2022
## [2.7.1](v2.7.0...v2.7.1) (2022-09-21)

### Reverts

* Revert "#138 - prevent a memory leak on AndroidView which may not call ON_DESTROY (#188)" (#202) ([5183ce0](5183ce0)), closes [#138](#138) [#188](#188) [#202](#202)
@googlemaps-bot
Copy link
Contributor

🎉 This PR is included in version 2.7.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@wangela
Copy link
Member

wangela commented Sep 21, 2022

So sorry @polivmi1, would you mind git commit --amend the commit message in your branch to:

fix: Prevent a memory leak on AndroidView which may not call ON_DESTROY

and then push the branch again? The reason for this is the original commit message did not follow conventional commit conventions, and was interpreted by our semantic release bot as not triggering a new release when in fact it is a bugfix and should trigger a patch release. I have reverted the merge from this PR so you can re-submit your changes.

Once you push the branch, it will create a new PR and you can reference this PR #188 and the original issue that is fixed #138 in the new PR's description, to link to the historical discussion preserved in this PR review.

Apologies for not catching the commit message format during the main review.

@polivmi1
Copy link
Contributor Author

@wangela I have changed the commit message in https://github.com/polivmi1/android-maps-compose/tree/138_memory_leak it hasn't created a new PR (this is already in state merged). Should I create a completely new PR and mention this one there?

@wangela
Copy link
Member

wangela commented Sep 21, 2022

@polivmi1 Yes, I see a "Create pull request" button here that you can use: main...polivmi1:android-maps-compose:138_memory_leak

@basurahan
Copy link

Waiting for this release to be fixed

@polivmi1
Copy link
Contributor Author

@wangela see #206

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.

Memory leak
6 participants