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

Don't remove dep keys in `didUnwatch` #17498

Merged
merged 1 commit into from Jan 22, 2019

Conversation

Projects
None yet
2 participants
@gitKrystan
Copy link
Contributor

gitKrystan commented Jan 22, 2019

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 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 #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 #17243

Co-authored-by: Godfrey Chan godfreykfc@gmail.com
(cherry picked from commit 1f8218b)

Don't remove dep keys in `didUnwatch`
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 `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 #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 #17243

Co-authored-by: Godfrey Chan <godfreykfc@gmail.com>
(cherry picked from commit 1f8218b)

@rwjblue rwjblue merged commit bcfad69 into emberjs:beta Jan 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Jan 22, 2019

Thank you!

@gitKrystan gitKrystan deleted the gitKrystan:backport-didunwatch-fix branch Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.