Fix unsafe reuse of broccoli trees in OneShot #1064
Merged
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
The OneShot transform consumes a broccoli tree a single time, detaching us permanently from future updates to that tree, which avoids needing to revalidate all the third-party v1-to-v2 addon conversion during rebuilds. Since OneShot fully consumes a broccoli tree in a separate broccoli pipeline, it's not safe to consume that same tree again in a different pipeline. But an addon can manipulate cacheKeyForTree such that two distinct copies of an addon emit the same broccoli tree instance as their v2Tree, which we send into OneShot. This fails in a very byzantine way. It's the source of the problems people have been having with test-waiters (#1056) because that addon aggressively tries to deduplicate itself. The fix is to not rerun OneShot if we encounter an identical tree a second time.
ef4
added a commit
that referenced
this pull request
Feb 1, 2022
The fix in #1064 turns out to be insufficient. The problem is that `cacheKeyForTree` can provide a cached tree at any point in the broccoli graph. We can't tell just by looking at the top level node for an addon whether it contains, deep down inside it, some trees that have already been run by other builders via OneShot. I don't like `cacheKeyForTree`. It's trying to solve a real problem, but at the wrong layer. Instead of sharing work between many addon instances, it should have been preventing there from *being* many addon instances in the first place. I'm moving Embroider to a solution more like that. We already have the `reduceInstances` hook, which is a more blunt and powerful way to through away whole Addon instances. We can give it a default implementation that will allow two addons with *entirely* identical cacheKeyForTree to deduplicate at the *instance* level, not the tree level.
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.
The OneShot transform consumes a broccoli tree a single time, detaching us permanently from future updates to that tree, which avoids needing to revalidate all the third-party v1-to-v2 addon conversion during rebuilds.
Since OneShot fully consumes a broccoli tree in a separate broccoli pipeline, it's not safe to consume that same tree again in a different pipeline. But an addon can manipulate cacheKeyForTree such that two distinct copies of an addon emit the same broccoli tree instance as their v2Tree, which we send into OneShot. This fails in a very byzantine way.
It's the source of the problems people have been having with test-waiters (#1056) because that addon aggressively tries to deduplicate itself.
The fix is to not rerun OneShot if we encounter an identical tree a second time.
(It would be nice to do away with OneShot but I think the best way to do that would be to rely on BROCCOLI_ENABLED_MEMOIZE, and I don't know when or if that feature is going to get enabled by default in broccoli.)