Don't remove dep keys in didUnwatch#17498
Merged
rwjblue merged 1 commit intoemberjs:betafrom Jan 22, 2019
Merged
Conversation
This fixes emberjs#17243 by only removing dependent keys in `teardown`. Since emberjs#16978 coalesces the work to add dependent keys to happen at most once, it is now incorrect to eagerly remove them in `didUnwatch` because that could happen for a variety of reasons while there are still other interested parties (see attached test cases). This limits the work of removing dependent keys to `teardown` only. The logic is that, while we want to defer setting up the "link" to the target property as lazily as possible, it is less important to "unlink" it eagerly once the work is done. It _may_ be possible to accomplish this, but the current amount of book-keeping is not sufficient to track the count properly. In our tests, we have created sequences like this: 1. setup (peekWatching = 0, so nothing happens) 2. get (consumed here) 3. willWatch (already consumed, no-op) 4. get (already consumed, no-op) 5. didUnwatch 6. ... In this case, it would be incorrect to "unlink" at step 5. As of PR emberjs#16978, `CONSUMED` is essentially a boolean flag, so it is not sufficient to track the balance, and also, there is no counterpart to `get`, which makes eager "unlinking" impossible without much more book-keeping. It's unclear that it would be worthwhile to do that. On the other hand, if we only "unlink" on teardown, this ensures that we are only "unlinking" at most once, and it is guaranteed that there are no longer any interested parties. Fixes emberjs#17243 Co-authored-by: Godfrey Chan <godfreykfc@gmail.com> (cherry picked from commit 1f8218b)
Member
|
Thank you! |
This file contains hidden or 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
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 fixes #17243 by only removing dependent keys in
teardown.Since #16978 coalesces the work to add dependent keys to happen at
most once, it is now incorrect to eagerly remove them in
didUnwatchbecause that could happen for a variety of reasons while there are
still other interested parties (see attached test cases).
This limits the work of removing dependent keys to
teardownonly.The logic is that, while we want to defer setting up the "link" to
the target property as lazily as possible, it is less important to
"unlink" it eagerly once the work is done. It may be possible to
accomplish this, but the current amount of book-keeping is not
sufficient to track the count properly. In our tests, we have
created sequences like this:
In this case, it would be incorrect to "unlink" at step 5. As of
PR #16978,
CONSUMEDis essentially a boolean flag, so it is notsufficient to track the balance, and also, there is no counterpart
to
get, which makes eager "unlinking" impossible without muchmore book-keeping. It's unclear that it would be worthwhile to do
that.
On the other hand, if we only "unlink" on teardown, this ensures
that we are only "unlinking" at most once, and it is guaranteed
that there are no longer any interested parties.
Fixes #17243
Co-authored-by: Godfrey Chan godfreykfc@gmail.com
(cherry picked from commit 1f8218b)