[BUGFIX lts] Ensure instantiation cannot happen after destruction. #18717
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This changes a bit about how destruction occurs and is enforced. Without these changes, it is possible to introduce memory leaks in FastBoot in a few (somewhat specialized) scenarios. Some examples: * if you were to call `owner.lookup` sometime after the owner itself was destroyed. During development and tests we prevent this (by way of an assertion), but in production we happily recreate the instances (without marking them as `isDestroying`) and never destroy them * if you were to call `owner.lookup` _during_ destruction (e.g. in another services `willDestroy`) we would emit no warning / assertion even in development mode, and we would never destroy the instances created In order to prevent these situations, this commit introduces these changes: 1. ensure that looking things up from the container _during_ destruction emits a deprecation 2. ensure that looking things up from the container _after_ destruction throws an error (**not** an assertion) 3. ensure that any objects that _are_ instantiated during container destruction (even though deprecated) will themselves be destroyed
pzuraq
approved these changes
Jan 31, 2020
krisselden
approved these changes
Jan 31, 2020
chancancode
added a commit
that referenced
this pull request
Feb 7, 2020
We agreed that it was important to fix the error cases in #18717, but while the work may be unnecessary, there isn't necessarily any "correctness" issues with looking up things during destruction. Using a deprecation to warn about potentially "unnecessary" work is a bit heavy-handed, and at minimum requires some more continued discussions, so reverting that part of the PR for now (while keeping the errors in tact).
chancancode
added a commit
that referenced
this pull request
Feb 8, 2020
We agreed that it was important to fix the error cases in #18717, but while the work may be unnecessary, there isn't necessarily any "correctness" issues with looking up things during destruction. Using a deprecation to warn about potentially "unnecessary" work is a bit heavy-handed, and at minimum requires some more continued discussions, so reverting that part of the PR for now (while keeping the errors in tact). (cherry picked from commit b853324)
chancancode
added a commit
that referenced
this pull request
Feb 8, 2020
We agreed that it was important to fix the error cases in #18717, but while the work may be unnecessary, there isn't necessarily any "correctness" issues with looking up things during destruction. Using a deprecation to warn about potentially "unnecessary" work is a bit heavy-handed, and at minimum requires some more continued discussions, so reverting that part of the PR for now (while keeping the errors in tact). (cherry picked from commit b853324)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes a bit about how destruction occurs and is enforced. Without these changes, it is possible to introduce memory leaks in FastBoot in a few (somewhat specialized) scenarios. Some examples:
owner.lookupsometime after the owner itself was destroyed. During development and tests we prevent this (by way of an assertion), but in production we happily recreate the instances (without marking them asisDestroying) and never destroy themowner.lookupduring destruction (e.g. in another serviceswillDestroy) we would emit no warning / assertion even in development mode, and we would never destroy the instances createdIn order to prevent these situations, this commit introduces these changes: