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

Fix unsafe reuse of broccoli trees in OneShot #1064

Merged
merged 1 commit into from
Jan 6, 2022
Merged

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Jan 6, 2022

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.)

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 ef4 merged commit 85e6743 into master Jan 6, 2022
@ef4 ef4 deleted the cached-one-shot branch January 6, 2022 23:22
@ef4 ef4 added the bug Something isn't working label Jan 8, 2022
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
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant