Add details about hidden leaks. #97
Conversation
doc/TROUBLESHOOT.md
Outdated
| And, there is no point to release reference to child inside the method `dispose` of the parent, because | ||
| not needed parent should be released together with its disposal. If | ||
| your fix for a leak is like this, you are defenitely hiding the leak: |
There was a problem hiding this comment.
I'm not sure this is true.
If the child is not nulled out, it may not be eligible for garbage collection (even if it has released some resource in dispose). And if it is not nulled out, other code in the class that depends on its nullability to determine whether it can be used or not may fail.
For example, it's pretty common in Flutter code to mark some native object as nullable, and once it is disposed null it out so it doesn't get used again.
There was a problem hiding this comment.
I am not saying it is wrong to null out. I am saying it is wrong to null out in dispose.
If parent is disposed, it should become non-used, unreachable and thus to be garbage collected together with child.
Another side effect of nulling in dispose is that you have to make the field nullable, that adds unnecessary complexity to the code and eliminates non-nullable optimizations.
How does it sound?
There was a problem hiding this comment.
Now, when leak_tracker checks that disposed members are garbage collected, this nulling out is not needed.
There was a problem hiding this comment.
Nulling it out in dispose helps with assertions and error checking that the parent and child objects are getting used correctly. It also helps avoid runtime errors due to misuse in some cases, where a null check can guard things even if asserts are disabled.
There was a problem hiding this comment.
I see. There are pros and cons nulling out member in dispose.
However, the statement here is: a leak fix should not be nulling out in dispose, because, while it makes the leak smaller, it does not fix it.
This means it is ok to null out the member in dispose in a separate PR, but not in order to fox the leak. Am I missing something?
There was a problem hiding this comment.
Yeah, I think pointing out that nulling it out isn't the right way to fix the leak, but I would avoid calling that style bad - it's used frequently in the flutter framework for the reasons I mentioned
There was a problem hiding this comment.
Modified. Is it better now?
Co-authored-by: Greg Spencer <gspencergoog@users.noreply.github.com>
No description provided.