From 5ec67ee2a7f355411111b008ad703afab36aaa9d Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Wed, 22 Jul 2015 22:45:54 -0700 Subject: [PATCH] fix(compiler): prevent race conditions Previously, the compiler would detect cycles where there were none just because of other components that were compiled in parallel. Furthermore, the way ProtoView merging was triggered could result into early exits resulting in errors when trying to instantiate ProtoViews. Fixes #3206 Closes #3211 --- .../angular2/src/core/compiler/compiler.ts | 177 ++++++++---------- .../src/core/compiler/proto_view_factory.ts | 4 +- modules/angular2/src/core/compiler/view.ts | 2 - .../test/core/compiler/compiler_spec.ts | 22 ++- .../compiler/projection_integration_spec.ts | 2 +- .../core/compiler/proto_view_factory_spec.ts | 6 +- 6 files changed, 107 insertions(+), 106 deletions(-) diff --git a/modules/angular2/src/core/compiler/compiler.ts b/modules/angular2/src/core/compiler/compiler.ts index ba7e4ef83e..8a454d6a0c 100644 --- a/modules/angular2/src/core/compiler/compiler.ts +++ b/modules/angular2/src/core/compiler/compiler.ts @@ -91,7 +91,6 @@ export class Compiler { private _appUrl: string; private _render: renderApi.RenderCompiler; private _protoViewFactory: ProtoViewFactory; - private _protoViewsToBeMerged: AppProtoView[] = []; /** * @private @@ -141,55 +140,28 @@ export class Compiler { hostPvPromise = this._render.compileHost(directiveMetadata) .then((hostRenderPv) => { - var protoView = this._protoViewFactory.createAppProtoViews( + var protoViews = this._protoViewFactory.createAppProtoViews( componentBinding, hostRenderPv, [componentBinding]); - this._compilerCache.setHost(componentType, protoView); - return this._compileNestedProtoViews(hostRenderPv, protoView, componentType); + return this._compileNestedProtoViews(protoViews, componentType, new Map()); + }) + .then((appProtoView) => { + this._compilerCache.setHost(componentType, appProtoView); + return appProtoView; }); } - return hostPvPromise.then(hostAppProtoView => - this._mergeUnmergedProtoViews().then(_ => hostAppProtoView.ref)); + return hostPvPromise.then(hostAppProtoView => hostAppProtoView.ref); } - private _mergeUnmergedProtoViews(): Promise { - var protoViewsToBeMerged = this._protoViewsToBeMerged; - this._protoViewsToBeMerged = []; - return PromiseWrapper.all(protoViewsToBeMerged.map((appProtoView) => { - return this._render.mergeProtoViewsRecursively( - this._collectMergeRenderProtoViews(appProtoView)) - .then((mergeResult: renderApi.RenderProtoViewMergeMapping) => { - appProtoView.mergeMapping = new AppProtoViewMergeMapping(mergeResult); - }); - })); - } - - private _collectMergeRenderProtoViews( - appProtoView: AppProtoView): List> { - var result = [appProtoView.render]; - for (var i = 0; i < appProtoView.elementBinders.length; i++) { - var binder = appProtoView.elementBinders[i]; - if (isPresent(binder.nestedProtoView)) { - if (binder.hasStaticComponent() || - (binder.hasEmbeddedProtoView() && binder.nestedProtoView.isEmbeddedFragment)) { - result.push(this._collectMergeRenderProtoViews(binder.nestedProtoView)); - } else { - result.push(null); - } - } - } - return result; - } - - private _compile(componentBinding: DirectiveBinding): Promise| AppProtoView { + private _compile(componentBinding: DirectiveBinding, + componentPath: Map): Promise| + AppProtoView { var component = componentBinding.key.token; var protoView = this._compilerCache.get(component); if (isPresent(protoView)) { // The component has already been compiled into an AppProtoView, - // returns a plain AppProtoView, not wrapped inside of a Promise. - // Needed for recursive components. + // returns a plain AppProtoView, not wrapped inside of a Promise, for performance reasons. return protoView; } - var resultPromise = this._compiling.get(component); if (isPresent(resultPromise)) { // The component is already being compiled, attach to the existing Promise @@ -212,17 +184,18 @@ export class Compiler { ListWrapper.map(directives, (directive) => this._bindDirective(directive))); var renderTemplate = this._buildRenderTemplate(component, view, boundDirectives); - resultPromise = this._render.compile(renderTemplate) - .then((renderPv) => { - var protoView = this._protoViewFactory.createAppProtoViews( - componentBinding, renderPv, boundDirectives); - // Populate the cache before compiling the nested components, - // so that components can reference themselves in their template. - this._compilerCache.set(component, protoView); - MapWrapper.delete(this._compiling, component); - - return this._compileNestedProtoViews(renderPv, protoView, component); - }); + resultPromise = + this._render.compile(renderTemplate) + .then((renderPv) => { + var protoViews = this._protoViewFactory.createAppProtoViews( + componentBinding, renderPv, boundDirectives); + return this._compileNestedProtoViews(protoViews, component, componentPath); + }) + .then((appProtoView) => { + this._compilerCache.set(component, appProtoView); + MapWrapper.delete(this._compiling, component); + return appProtoView; + }); this._compiling.set(component, resultPromise); return resultPromise; } @@ -233,67 +206,83 @@ export class Compiler { return MapWrapper.values(directivesMap); } - private _compileNestedProtoViews(renderProtoView: renderApi.ProtoViewDto, - appProtoView: AppProtoView, - componentType: Type): Promise { + private _compileNestedProtoViews(appProtoViews: AppProtoView[], componentType: Type, + componentPath: Map): Promise { var nestedPVPromises = []; - this._loopComponentElementBinders(appProtoView, (parentPv, elementBinder: ElementBinder) => { - var nestedComponent = elementBinder.componentDirective; - var elementBinderDone = - (nestedPv: AppProtoView) => { elementBinder.nestedProtoView = nestedPv; }; - var nestedCall = this._compile(nestedComponent); - if (isPromise(nestedCall)) { - nestedPVPromises.push((>nestedCall).then(elementBinderDone)); - } else { - elementBinderDone(nestedCall); - } + componentPath = MapWrapper.clone(componentPath); + if (appProtoViews[0].type === renderApi.ViewType.COMPONENT) { + componentPath.set(componentType, appProtoViews[0]); + } + appProtoViews.forEach(appProtoView => { + this._collectComponentElementBinders(appProtoView) + .forEach((elementBinder: ElementBinder) => { + var nestedComponent = elementBinder.componentDirective; + var nestedComponentType = nestedComponent.key.token; + var elementBinderDone = + (nestedPv: AppProtoView) => { elementBinder.nestedProtoView = nestedPv; }; + if (componentPath.has(nestedComponentType)) { + // cycle... + if (appProtoView.isEmbeddedFragment) { + throw new BaseException( + ` is used within the recursive path of ${stringify(nestedComponentType)}`); + } else if (appProtoView.type === renderApi.ViewType.COMPONENT) { + throw new BaseException( + `Unconditional component cycle in ${stringify(nestedComponentType)}`); + } else { + elementBinderDone(componentPath.get(nestedComponentType)); + } + } else { + var nestedCall = this._compile(nestedComponent, componentPath); + if (isPromise(nestedCall)) { + nestedPVPromises.push((>nestedCall).then(elementBinderDone)); + } else { + elementBinderDone(nestedCall); + } + } + }); }); return PromiseWrapper.all(nestedPVPromises) - .then((_) => { - this._collectMergableProtoViews(appProtoView, componentType); - return appProtoView; + .then(_ => PromiseWrapper.all( + appProtoViews.map(appProtoView => this._mergeProtoView(appProtoView)))) + .then(_ => appProtoViews[0]); + } + + private _mergeProtoView(appProtoView: AppProtoView): Promise { + if (appProtoView.type !== renderApi.ViewType.HOST && + appProtoView.type !== renderApi.ViewType.EMBEDDED) { + return null; + } + return this._render.mergeProtoViewsRecursively(this._collectMergeRenderProtoViews(appProtoView)) + .then((mergeResult: renderApi.RenderProtoViewMergeMapping) => { + appProtoView.mergeMapping = new AppProtoViewMergeMapping(mergeResult); }); } - private _collectMergableProtoViews(appProtoView: AppProtoView, componentType: Type) { - var isRecursive = false; + private _collectMergeRenderProtoViews( + appProtoView: AppProtoView): List> { + var result = [appProtoView.render]; for (var i = 0; i < appProtoView.elementBinders.length; i++) { var binder = appProtoView.elementBinders[i]; - if (binder.hasStaticComponent()) { - if (isBlank(binder.nestedProtoView.isRecursive)) { - // cycle via a component. We are in the tail recursion, - // so all components should have their isRecursive flag set already. - isRecursive = true; - break; + if (isPresent(binder.nestedProtoView)) { + if (binder.hasStaticComponent() || + (binder.hasEmbeddedProtoView() && binder.nestedProtoView.isEmbeddedFragment)) { + result.push(this._collectMergeRenderProtoViews(binder.nestedProtoView)); + } else { + result.push(null); } - } else if (binder.hasEmbeddedProtoView()) { - this._collectMergableProtoViews(binder.nestedProtoView, componentType); } } - if (isRecursive) { - if (appProtoView.isEmbeddedFragment) { - throw new BaseException( - ` is used within the recursive path of ${stringify(componentType)}`); - } - if (appProtoView.type === renderApi.ViewType.COMPONENT) { - throw new BaseException(`Unconditional component cycle in ${stringify(componentType)}`); - } - } - if (appProtoView.type === renderApi.ViewType.EMBEDDED || - appProtoView.type === renderApi.ViewType.HOST) { - this._protoViewsToBeMerged.push(appProtoView); - } - appProtoView.isRecursive = isRecursive; + return result; } - private _loopComponentElementBinders(appProtoView: AppProtoView, callback: Function) { + private _collectComponentElementBinders(appProtoView: AppProtoView): ElementBinder[] { + var componentElementBinders = []; appProtoView.elementBinders.forEach((elementBinder) => { if (isPresent(elementBinder.componentDirective)) { - callback(appProtoView, elementBinder); - } else if (isPresent(elementBinder.nestedProtoView)) { - this._loopComponentElementBinders(elementBinder.nestedProtoView, callback); + componentElementBinders.push(elementBinder); } }); + return componentElementBinders; } private _buildRenderTemplate(component, view, directives): renderApi.ViewDefinition { diff --git a/modules/angular2/src/core/compiler/proto_view_factory.ts b/modules/angular2/src/core/compiler/proto_view_factory.ts index abb3d81edf..fbecda65e2 100644 --- a/modules/angular2/src/core/compiler/proto_view_factory.ts +++ b/modules/angular2/src/core/compiler/proto_view_factory.ts @@ -160,7 +160,7 @@ export class ProtoViewFactory { createAppProtoViews(hostComponentBinding: DirectiveBinding, rootRenderProtoView: renderApi.ProtoViewDto, - allDirectives: List): AppProtoView { + allDirectives: List): AppProtoView[] { var allRenderDirectiveMetadata = ListWrapper.map(allDirectives, directiveBinding => directiveBinding.metadata); var nestedPvsWithIndex = _collectNestedProtoViews(rootRenderProtoView); @@ -184,7 +184,7 @@ export class ProtoViewFactory { } appProtoViews[pvWithIndex.index] = appProtoView; }); - return appProtoViews[0]; + return appProtoViews; } } diff --git a/modules/angular2/src/core/compiler/view.ts b/modules/angular2/src/core/compiler/view.ts index e571551637..9237d2de44 100644 --- a/modules/angular2/src/core/compiler/view.ts +++ b/modules/angular2/src/core/compiler/view.ts @@ -261,8 +261,6 @@ export class AppProtoView { mergeMapping: AppProtoViewMergeMapping; ref: ProtoViewRef; - isRecursive: boolean = null; - constructor(public type: renderApi.ViewType, public isEmbeddedFragment: boolean, public render: renderApi.RenderProtoViewRef, public protoChangeDetector: ProtoChangeDetector, diff --git a/modules/angular2/test/core/compiler/compiler_spec.ts b/modules/angular2/test/core/compiler/compiler_spec.ts index 44560d668e..e4e3bb3a17 100644 --- a/modules/angular2/test/core/compiler/compiler_spec.ts +++ b/modules/angular2/test/core/compiler/compiler_spec.ts @@ -374,7 +374,7 @@ export function main() { it('should cache compiled host components', inject([AsyncTestCompleter], (async) => { tplResolver.setView(MainComponent, new viewAnn.View({template: '
'})); var mainPv = createProtoView(); - var compiler = createCompiler([createRenderProtoView()], [rootProtoView, mainPv]); + var compiler = createCompiler([createRenderProtoView([])], [rootProtoView, mainPv]); compiler.compileInHost(MainComponent) .then((protoViewRef) => { expect(internalProtoView(protoViewRef).elementBinders[0].nestedProtoView) @@ -535,7 +535,8 @@ export function main() { it('should create host proto views', inject([AsyncTestCompleter], (async) => { tplResolver.setView(MainComponent, new viewAnn.View({template: '
'})); var rootProtoView = - createProtoView([createComponentElementBinder(directiveResolver, MainComponent)]); + createProtoView([createComponentElementBinder(directiveResolver, MainComponent)], + renderApi.ViewType.HOST); var mainProtoView = createProtoView(); var compiler = createCompiler([createRenderProtoView()], [rootProtoView, mainProtoView]); compiler.compileInHost(MainComponent) @@ -694,9 +695,9 @@ class FakeProtoViewFactory extends ProtoViewFactory { } createAppProtoViews(componentBinding: DirectiveBinding, renderProtoView: renderApi.ProtoViewDto, - directives: List): AppProtoView { + directives: List): AppProtoView[] { this.requests.push([componentBinding, renderProtoView, directives]); - return ListWrapper.removeAt(this.results, 0); + return collectEmbeddedPvs(ListWrapper.removeAt(this.results, 0)); } } @@ -706,4 +707,17 @@ class MergedRenderProtoViewRef extends renderApi.RenderProtoViewRef { function originalRenderProtoViewRefs(appProtoView: AppProtoView) { return (appProtoView.mergeMapping.renderProtoViewRef).originals; +} + +function collectEmbeddedPvs(pv: AppProtoView, target: AppProtoView[] = null): AppProtoView[] { + if (isBlank(target)) { + target = []; + } + target.push(pv); + pv.elementBinders.forEach(elementBinder => { + if (elementBinder.hasEmbeddedProtoView()) { + collectEmbeddedPvs(elementBinder.nestedProtoView, target); + } + }); + return target; } \ No newline at end of file diff --git a/modules/angular2/test/core/compiler/projection_integration_spec.ts b/modules/angular2/test/core/compiler/projection_integration_spec.ts index 822fa1eb5b..de57bcdd09 100644 --- a/modules/angular2/test/core/compiler/projection_integration_spec.ts +++ b/modules/angular2/test/core/compiler/projection_integration_spec.ts @@ -486,4 +486,4 @@ class Tab { }) class Tree { depth = 0; -} \ No newline at end of file +} diff --git a/modules/angular2/test/core/compiler/proto_view_factory_spec.ts b/modules/angular2/test/core/compiler/proto_view_factory_spec.ts index d056fd320f..9c2809e535 100644 --- a/modules/angular2/test/core/compiler/proto_view_factory_spec.ts +++ b/modules/angular2/test/core/compiler/proto_view_factory_spec.ts @@ -66,10 +66,10 @@ export function main() { var varBindings = new Map(); varBindings.set('a', 'b'); var renderPv = createRenderProtoView([], null, varBindings); - var appPv = + var appPvs = protoViewFactory.createAppProtoViews(bindDirective(MainComponent), renderPv, []); - expect(appPv.variableBindings.get('a')).toEqual('b'); - expect(appPv).toBeTruthy(); + expect(appPvs[0].variableBindings.get('a')).toEqual('b'); + expect(appPvs.length).toBe(1); }); });