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

Failing test and fix for #17243 #17487

Merged
merged 2 commits into from Jan 18, 2019

Conversation

gitKrystan
Copy link
Contributor

@gitKrystan gitKrystan commented Jan 16, 2019

Previously #16978 tried to balance the amount of addDependentKeys and removeDependentKeys. However, it removes the dependent keys too eagerly, causing a different kind of imbalance, and in some cases, this causes the property tags to "miss" updates to aliased properties.

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

gitKrystan and others added 2 commits January 16, 2019 15:00
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>
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 gitKrystan changed the title [WIP] Failing test and fix for #17243 Failing test and fix for #17243 Jan 17, 2019
@chancancode
Copy link
Member

cc @bekzod

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.

Thank you for digging in here! I really appreciate the detailed explanation of the problem.

@bekzod
Copy link
Contributor

bekzod commented Jan 18, 2019

Looks good 👍

@chancancode chancancode merged commit 726c4e2 into emberjs:master Jan 18, 2019
@chancancode
Copy link
Member

@rwjblue should this be back ported beta/release/lts?

@rwjblue
Copy link
Member

rwjblue commented Jan 18, 2019

@chancancode yes, but we need to confirm the first version that had the issue (to make sure we don't go back too far)

@gitKrystan gitKrystan deleted the failing-test-17243-redux branch January 18, 2019 17:06
@gitKrystan
Copy link
Contributor Author

I deployed master with this change, and #17243 is fixed in our app. 👌

@chancancode
Copy link
Member

@gitKrystan do we know which version first broke this?

@gitKrystan
Copy link
Contributor Author

@chancancode First seen in 3.5.0-beta.3

@jasonmit
Copy link
Member

Thanks a ton for working on this. This bug is also breaking our app currently.

@ginomiglio
Copy link

We're also having issues with this after upgrading to 3.5. Any news on backporting?

@chancancode
Copy link
Member

@ginomiglio I believe this ended up landing in 3.8. Seeing that we are about to release 3.9 at this point, and that 3.8 is going to become a LTS release, I don't think we would consider backporting it further back. Are there any issues preventing you from upgrading to 3.8?

@ginomiglio
Copy link

ginomiglio commented Mar 29, 2019

Ah, ok. It'll just force us to upgrade from 3.4 -> 3.8 directly instead of one version at a time, which will be...fun for our enormous monolithic Ember app 😅

@chancancode
Copy link
Member

chancancode commented Mar 29, 2019

@ginomiglio since both 3.4 and 3.8 are LTS releases, it is actually recommended that you go from the latest 3.4 release to the 3.8 release, since they should be better maintained than the point releases in between. (This bug is actually a good example of that.) If you ran into other issues, please do report them!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ember.computed.oneWay teardown is "resetting" the property in an inner component
6 participants