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.

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

Unexpected behavior with retained instance notification #1427

Closed
pyricau opened this issue Jul 2, 2019 · 3 comments · Fixed by #1433
Closed

Unexpected behavior with retained instance notification #1427

pyricau opened this issue Jul 2, 2019 · 3 comments · Fixed by #1433
Labels
type: bug
Milestone
2.0-alpha-3

Comments

@pyricau
Copy link
Member

pyricau commented Jul 2, 2019

  • Notification shows up with 2/5 leaks
  • Then it disappears after 10 seconds
  • If tapping it in time, it goes away and nothing happens. Logcat says "No retained instances after GC"

Combination of issues there:

  • Hiding the notification when the count goes to 0 isn't great because people wonder where it went.
  • If it consistently gets "No retained instances after GC" when tapping it, then we're either not refreshing often enough, or we should run the GC more often.
  • Tapping it should show another notification that says there was no retained instances. Sounds like that's not happening, which would be a bug.
@pyricau pyricau added this to the 2.0 Next Release milestone Jul 2, 2019
@pforhan
Copy link

pforhan commented Jul 2, 2019

related: if you leave the app idle LC will keep reporting the retained instances basically indefinitely. Triggering a gc with the notification or by causing the app to do so will of course drop these down to 0 (and cause the aforementioned disappearance)

@pyricau
Copy link
Member Author

pyricau commented Jul 3, 2019 •
edited
Loading

Reproduced!

  • When we tap the notification and a GC leads to no leak left, we do display the "No Retained Instance" notification, however it's quickly dismissed because a check had already been enqueued and when that runs it finds no retained instances and dismisses it => We should use a distinct notification id for that specific case.
  • Once we have retained instances, we should check more often (currently 5 seconds). Let's move to every 2 seconds. That way the notification is more fresh.
  • When the count drops to 0, we should keep a notification for a while to let users know. "All retained instances were garbage collected. This notification will disappear after 30 seconds"

@pyricau
Copy link
Member Author

pyricau commented Jul 3, 2019

Found an additional bug, we're not running the GC when under the threshold of 5.

pyricau added a commit that referenced this issue Jul 3, 2019
@pyricau
Behavior change & bugfixes for retained count notification
4c3a394 
* Bugfix: when tapping the "dump heap" notification, if the GC found no retained instance we didn't use a distinct notification id to communicate that, and then soon after we'd dismiss the notification that let's the user know there is no retained instances on tap.
* Bugfix: we weren't running the GC when checking for retained instances and the count was under the threshold
* Change: we now update the notification and keep it around for 30 seconds when the count goes to 0
* Change: the "no retained instance" notification that shows on tap is identical and also auto dismisses after 30 seconds.
* Change: we're checking for retained references (and running the GC) every 2 seconds instead of 5.

Fixes #1427
@pyricau pyricau mentioned this issue Jul 3, 2019
Merged
pyricau added a commit that referenced this issue Jul 3, 2019
@pyricau
Behavior change & bugfixes for retained count notification
095a3dd 
* Bugfix: when tapping the "dump heap" notification, if the GC found no retained instance we didn't use a distinct notification id to communicate that, and then soon after we'd dismiss the notification that let's the user know there is no retained instances on tap.
* Bugfix: we weren't running the GC when checking for retained instances and the count was under the threshold
* Change: we now update the notification and keep it around for 30 seconds when the count goes to 0
* Change: the "no retained instance" notification that shows on tap is identical and also auto dismisses after 30 seconds.
* Change: we're checking for retained references (and running the GC) every 2 seconds instead of 5.

Fixes #1427
pyricau added a commit that referenced this issue Jul 3, 2019
@pyricau
Behavior change & bugfixes for retained count notification
ff131ba 
* Bugfix: when tapping the "dump heap" notification, if the GC found no retained instance we didn't use a distinct notification id to communicate that, and then soon after we'd dismiss the notification that let's the user know there is no retained instances on tap.
* Bugfix: we weren't running the GC when checking for retained instances and the count was under the threshold
* Change: we now update the notification and keep it around for 30 seconds when the count goes to 0
* Change: the "no retained instance" notification that shows on tap is identical and also auto dismisses after 30 seconds.
* Change: we're checking for retained references (and running the GC) every 2 seconds instead of 5.

Fixes #1427
pyricau added a commit that referenced this issue Jul 3, 2019
@pyricau
Behavior change & bugfixes for retained count notification (#1433)
968083a 
* Bugfix: when tapping the "dump heap" notification, if the GC found no retained instance we didn't use a distinct notification id to communicate that, and then soon after we'd dismiss the notification that let's the user know there is no retained instances on tap.
* Bugfix: we weren't running the GC when checking for retained instances and the count was under the threshold
* Change: we now update the notification and keep it around for 30 seconds when the count goes to 0
* Change: the "no retained instance" notification that shows on tap is identical and also auto dismisses after 30 seconds.
* Change: we're checking for retained references (and running the GC) every 2 seconds instead of 5.

Fixes #1427
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: bug
Projects
None yet
Milestone
2.0-alpha-3
Development

Successfully merging a pull request may close this issue.

Behavior change & bugfixes for retained count notification square/leakcanary
2 participants
@pforhan @pyricau