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

Jump to bottom

[BUGFIX beta] properly teardown alias #16978

Merged
merged 1 commit into from Sep 21, 2018
Merged

[BUGFIX beta] properly teardown alias #16978

merged 1 commit into from Sep 21, 2018

Conversation

bekzod
Copy link
Contributor

@bekzod bekzod commented Sep 14, 2018

attempt to balance consumed/unconsumed properties in Alias,
in some cases Alias wouldn't teardown properly leaving unbalanced watch count in meta

  1. alias created => get called on alias => alias destroyed (still watching on original prop)
  2. alias created => observer added on alias => alias destroyed (still watching on original prop)

@bekzod bekzod force-pushed the teardown-alias branch 2 times, most recently from 68315c4 to 96e5075 Compare September 14, 2018 20:40
rwjblue
rwjblue approved these changes Sep 16, 2018
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good to me, thanks for working on it!

Can you prefix the commit and PR with [BUGFIX beta]?

@bekzod bekzod changed the title [BUGFIX] properly teardown alias [BUGFIX beta] properly teardown alias Sep 17, 2018
@rwjblue
Copy link
Member

rwjblue commented Sep 17, 2018

Thanks for updating the commit title, just waiting on @krisselden's review now...

@krisselden krisselden merged commit abe7c35 into emberjs:master Sep 21, 2018
@bekzod bekzod deleted the teardown-alias branch October 9, 2018 07:06
@gitKrystan gitKrystan mentioned this pull request Dec 3, 2018
Closed
gitKrystan added a commit to gitKrystan/ember.js that referenced this pull request Jan 16, 2019
@gitKrystan @chancancode
Failing test for emberjs#17243
6267e7e 
Previously emberjs#16978 tried to balance the amount of `addDependentKeys`
and `removeDependentKeys`. However, it removes the dependent keys
to eagerly, causing a different kind of imbalance, and in some cases,
this causes the property tags to "miss" updates to aliased properties.

Co-authored-by: Godfrey Chan <godfreykfc@gmail.com>
@gitKrystan gitKrystan mentioned this pull request Jan 16, 2019
Merged
gitKrystan added a commit to gitKrystan/ember.js that referenced this pull request Jan 17, 2019
@gitKrystan
Don't remove dep keys in didUnwatch
c3c6284 
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
gitKrystan added a commit to gitKrystan/ember.js that referenced this pull request Jan 17, 2019
@gitKrystan @chancancode
Don't remove dep keys in didUnwatch
1f8218b 
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>
gitKrystan added a commit to gitKrystan/ember.js that referenced this pull request Jan 22, 2019
@gitKrystan
Don't remove dep keys in didUnwatch
3990377 
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)
@gitKrystan gitKrystan mentioned this pull request Jan 22, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

@krisselden krisselden krisselden approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bekzod @rwjblue @wagenet @krisselden