X Tutup
Skip to content

Commit 5ec67ee

Browse files
tboschtbosch
committed
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
1 parent 61b7703 commit 5ec67ee

File tree

6 files changed

+107
-106
lines changed

6 files changed

+107
-106
lines changed

‎modules/angular2/src/core/compiler/compiler.ts‎

Lines changed: 83 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,6 @@ export class Compiler {
9191
private _appUrl: string;
9292
private _render: renderApi.RenderCompiler;
9393
private _protoViewFactory: ProtoViewFactory;
94-
private _protoViewsToBeMerged: AppProtoView[] = [];
9594

9695
/**
9796
* @private
@@ -141,55 +140,28 @@ export class Compiler {
141140
hostPvPromise =
142141
this._render.compileHost(directiveMetadata)
143142
.then((hostRenderPv) => {
144-
var protoView = this._protoViewFactory.createAppProtoViews(
143+
var protoViews = this._protoViewFactory.createAppProtoViews(
145144
componentBinding, hostRenderPv, [componentBinding]);
146-
this._compilerCache.setHost(componentType, protoView);
147-
return this._compileNestedProtoViews(hostRenderPv, protoView, componentType);
145+
return this._compileNestedProtoViews(protoViews, componentType, new Map());
146+
})
147+
.then((appProtoView) => {
148+
this._compilerCache.setHost(componentType, appProtoView);
149+
return appProtoView;
148150
});
149151
}
150-
return hostPvPromise.then(hostAppProtoView =>
151-
this._mergeUnmergedProtoViews().then(_ => hostAppProtoView.ref));
152+
return hostPvPromise.then(hostAppProtoView => hostAppProtoView.ref);
152153
}
153154

154-
private _mergeUnmergedProtoViews(): Promise<any> {
155-
var protoViewsToBeMerged = this._protoViewsToBeMerged;
156-
this._protoViewsToBeMerged = [];
157-
return PromiseWrapper.all(protoViewsToBeMerged.map((appProtoView) => {
158-
return this._render.mergeProtoViewsRecursively(
159-
this._collectMergeRenderProtoViews(appProtoView))
160-
.then((mergeResult: renderApi.RenderProtoViewMergeMapping) => {
161-
appProtoView.mergeMapping = new AppProtoViewMergeMapping(mergeResult);
162-
});
163-
}));
164-
}
165-
166-
private _collectMergeRenderProtoViews(
167-
appProtoView: AppProtoView): List<renderApi.RenderProtoViewRef | List<any>> {
168-
var result = [appProtoView.render];
169-
for (var i = 0; i < appProtoView.elementBinders.length; i++) {
170-
var binder = appProtoView.elementBinders[i];
171-
if (isPresent(binder.nestedProtoView)) {
172-
if (binder.hasStaticComponent() ||
173-
(binder.hasEmbeddedProtoView() && binder.nestedProtoView.isEmbeddedFragment)) {
174-
result.push(this._collectMergeRenderProtoViews(binder.nestedProtoView));
175-
} else {
176-
result.push(null);
177-
}
178-
}
179-
}
180-
return result;
181-
}
182-
183-
private _compile(componentBinding: DirectiveBinding): Promise<AppProtoView>| AppProtoView {
155+
private _compile(componentBinding: DirectiveBinding,
156+
componentPath: Map<Type, AppProtoView>): Promise<AppProtoView>|
157+
AppProtoView {
184158
var component = <Type>componentBinding.key.token;
185159
var protoView = this._compilerCache.get(component);
186160
if (isPresent(protoView)) {
187161
// The component has already been compiled into an AppProtoView,
188-
// returns a plain AppProtoView, not wrapped inside of a Promise.
189-
// Needed for recursive components.
162+
// returns a plain AppProtoView, not wrapped inside of a Promise, for performance reasons.
190163
return protoView;
191164
}
192-
193165
var resultPromise = this._compiling.get(component);
194166
if (isPresent(resultPromise)) {
195167
// The component is already being compiled, attach to the existing Promise
@@ -212,17 +184,18 @@ export class Compiler {
212184
ListWrapper.map(directives, (directive) => this._bindDirective(directive)));
213185

214186
var renderTemplate = this._buildRenderTemplate(component, view, boundDirectives);
215-
resultPromise = this._render.compile(renderTemplate)
216-
.then((renderPv) => {
217-
var protoView = this._protoViewFactory.createAppProtoViews(
218-
componentBinding, renderPv, boundDirectives);
219-
// Populate the cache before compiling the nested components,
220-
// so that components can reference themselves in their template.
221-
this._compilerCache.set(component, protoView);
222-
MapWrapper.delete(this._compiling, component);
223-
224-
return this._compileNestedProtoViews(renderPv, protoView, component);
225-
});
187+
resultPromise =
188+
this._render.compile(renderTemplate)
189+
.then((renderPv) => {
190+
var protoViews = this._protoViewFactory.createAppProtoViews(
191+
componentBinding, renderPv, boundDirectives);
192+
return this._compileNestedProtoViews(protoViews, component, componentPath);
193+
})
194+
.then((appProtoView) => {
195+
this._compilerCache.set(component, appProtoView);
196+
MapWrapper.delete(this._compiling, component);
197+
return appProtoView;
198+
});
226199
this._compiling.set(component, resultPromise);
227200
return resultPromise;
228201
}
@@ -233,67 +206,83 @@ export class Compiler {
233206
return MapWrapper.values(directivesMap);
234207
}
235208

236-
private _compileNestedProtoViews(renderProtoView: renderApi.ProtoViewDto,
237-
appProtoView: AppProtoView,
238-
componentType: Type): Promise<AppProtoView> {
209+
private _compileNestedProtoViews(appProtoViews: AppProtoView[], componentType: Type,
210+
componentPath: Map<Type, AppProtoView>): Promise<AppProtoView> {
239211
var nestedPVPromises = [];
240-
this._loopComponentElementBinders(appProtoView, (parentPv, elementBinder: ElementBinder) => {
241-
var nestedComponent = elementBinder.componentDirective;
242-
var elementBinderDone =
243-
(nestedPv: AppProtoView) => { elementBinder.nestedProtoView = nestedPv; };
244-
var nestedCall = this._compile(nestedComponent);
245-
if (isPromise(nestedCall)) {
246-
nestedPVPromises.push((<Promise<AppProtoView>>nestedCall).then(elementBinderDone));
247-
} else {
248-
elementBinderDone(<AppProtoView>nestedCall);
249-
}
212+
componentPath = MapWrapper.clone(componentPath);
213+
if (appProtoViews[0].type === renderApi.ViewType.COMPONENT) {
214+
componentPath.set(componentType, appProtoViews[0]);
215+
}
216+
appProtoViews.forEach(appProtoView => {
217+
this._collectComponentElementBinders(appProtoView)
218+
.forEach((elementBinder: ElementBinder) => {
219+
var nestedComponent = elementBinder.componentDirective;
220+
var nestedComponentType = <Type>nestedComponent.key.token;
221+
var elementBinderDone =
222+
(nestedPv: AppProtoView) => { elementBinder.nestedProtoView = nestedPv; };
223+
if (componentPath.has(nestedComponentType)) {
224+
// cycle...
225+
if (appProtoView.isEmbeddedFragment) {
226+
throw new BaseException(
227+
`<ng-content> is used within the recursive path of ${stringify(nestedComponentType)}`);
228+
} else if (appProtoView.type === renderApi.ViewType.COMPONENT) {
229+
throw new BaseException(
230+
`Unconditional component cycle in ${stringify(nestedComponentType)}`);
231+
} else {
232+
elementBinderDone(componentPath.get(nestedComponentType));
233+
}
234+
} else {
235+
var nestedCall = this._compile(nestedComponent, componentPath);
236+
if (isPromise(nestedCall)) {
237+
nestedPVPromises.push((<Promise<AppProtoView>>nestedCall).then(elementBinderDone));
238+
} else {
239+
elementBinderDone(<AppProtoView>nestedCall);
240+
}
241+
}
242+
});
250243
});
251244
return PromiseWrapper.all(nestedPVPromises)
252-
.then((_) => {
253-
this._collectMergableProtoViews(appProtoView, componentType);
254-
return appProtoView;
245+
.then(_ => PromiseWrapper.all(
246+
appProtoViews.map(appProtoView => this._mergeProtoView(appProtoView))))
247+
.then(_ => appProtoViews[0]);
248+
}
249+
250+
private _mergeProtoView(appProtoView: AppProtoView): Promise<any> {
251+
if (appProtoView.type !== renderApi.ViewType.HOST &&
252+
appProtoView.type !== renderApi.ViewType.EMBEDDED) {
253+
return null;
254+
}
255+
return this._render.mergeProtoViewsRecursively(this._collectMergeRenderProtoViews(appProtoView))
256+
.then((mergeResult: renderApi.RenderProtoViewMergeMapping) => {
257+
appProtoView.mergeMapping = new AppProtoViewMergeMapping(mergeResult);
255258
});
256259
}
257260

258-
private _collectMergableProtoViews(appProtoView: AppProtoView, componentType: Type) {
259-
var isRecursive = false;
261+
private _collectMergeRenderProtoViews(
262+
appProtoView: AppProtoView): List<renderApi.RenderProtoViewRef | List<any>> {
263+
var result = [appProtoView.render];
260264
for (var i = 0; i < appProtoView.elementBinders.length; i++) {
261265
var binder = appProtoView.elementBinders[i];
262-
if (binder.hasStaticComponent()) {
263-
if (isBlank(binder.nestedProtoView.isRecursive)) {
264-
// cycle via a component. We are in the tail recursion,
265-
// so all components should have their isRecursive flag set already.
266-
isRecursive = true;
267-
break;
266+
if (isPresent(binder.nestedProtoView)) {
267+
if (binder.hasStaticComponent() ||
268+
(binder.hasEmbeddedProtoView() && binder.nestedProtoView.isEmbeddedFragment)) {
269+
result.push(this._collectMergeRenderProtoViews(binder.nestedProtoView));
270+
} else {
271+
result.push(null);
268272
}
269-
} else if (binder.hasEmbeddedProtoView()) {
270-
this._collectMergableProtoViews(binder.nestedProtoView, componentType);
271-
}
272-
}
273-
if (isRecursive) {
274-
if (appProtoView.isEmbeddedFragment) {
275-
throw new BaseException(
276-
`<ng-content> is used within the recursive path of ${stringify(componentType)}`);
277-
}
278-
if (appProtoView.type === renderApi.ViewType.COMPONENT) {
279-
throw new BaseException(`Unconditional component cycle in ${stringify(componentType)}`);
280273
}
281274
}
282-
if (appProtoView.type === renderApi.ViewType.EMBEDDED ||
283-
appProtoView.type === renderApi.ViewType.HOST) {
284-
this._protoViewsToBeMerged.push(appProtoView);
285-
}
286-
appProtoView.isRecursive = isRecursive;
275+
return result;
287276
}
288277

289-
private _loopComponentElementBinders(appProtoView: AppProtoView, callback: Function) {
278+
private _collectComponentElementBinders(appProtoView: AppProtoView): ElementBinder[] {
279+
var componentElementBinders = [];
290280
appProtoView.elementBinders.forEach((elementBinder) => {
291281
if (isPresent(elementBinder.componentDirective)) {
292-
callback(appProtoView, elementBinder);
293-
} else if (isPresent(elementBinder.nestedProtoView)) {
294-
this._loopComponentElementBinders(elementBinder.nestedProtoView, callback);
282+
componentElementBinders.push(elementBinder);
295283
}
296284
});
285+
return componentElementBinders;
297286
}
298287

299288
private _buildRenderTemplate(component, view, directives): renderApi.ViewDefinition {

‎modules/angular2/src/core/compiler/proto_view_factory.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ export class ProtoViewFactory {
160160

161161
createAppProtoViews(hostComponentBinding: DirectiveBinding,
162162
rootRenderProtoView: renderApi.ProtoViewDto,
163-
allDirectives: List<DirectiveBinding>): AppProtoView {
163+
allDirectives: List<DirectiveBinding>): AppProtoView[] {
164164
var allRenderDirectiveMetadata =
165165
ListWrapper.map(allDirectives, directiveBinding => directiveBinding.metadata);
166166
var nestedPvsWithIndex = _collectNestedProtoViews(rootRenderProtoView);
@@ -184,7 +184,7 @@ export class ProtoViewFactory {
184184
}
185185
appProtoViews[pvWithIndex.index] = appProtoView;
186186
});
187-
return appProtoViews[0];
187+
return appProtoViews;
188188
}
189189
}
190190

‎modules/angular2/src/core/compiler/view.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -261,8 +261,6 @@ export class AppProtoView {
261261
mergeMapping: AppProtoViewMergeMapping;
262262
ref: ProtoViewRef;
263263

264-
isRecursive: boolean = null;
265-
266264
constructor(public type: renderApi.ViewType, public isEmbeddedFragment: boolean,
267265
public render: renderApi.RenderProtoViewRef,
268266
public protoChangeDetector: ProtoChangeDetector,

‎modules/angular2/test/core/compiler/compiler_spec.ts‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,7 @@ export function main() {
374374
it('should cache compiled host components', inject([AsyncTestCompleter], (async) => {
375375
tplResolver.setView(MainComponent, new viewAnn.View({template: '<div></div>'}));
376376
var mainPv = createProtoView();
377-
var compiler = createCompiler([createRenderProtoView()], [rootProtoView, mainPv]);
377+
var compiler = createCompiler([createRenderProtoView([])], [rootProtoView, mainPv]);
378378
compiler.compileInHost(MainComponent)
379379
.then((protoViewRef) => {
380380
expect(internalProtoView(protoViewRef).elementBinders[0].nestedProtoView)
@@ -535,7 +535,8 @@ export function main() {
535535
it('should create host proto views', inject([AsyncTestCompleter], (async) => {
536536
tplResolver.setView(MainComponent, new viewAnn.View({template: '<div></div>'}));
537537
var rootProtoView =
538-
createProtoView([createComponentElementBinder(directiveResolver, MainComponent)]);
538+
createProtoView([createComponentElementBinder(directiveResolver, MainComponent)],
539+
renderApi.ViewType.HOST);
539540
var mainProtoView = createProtoView();
540541
var compiler = createCompiler([createRenderProtoView()], [rootProtoView, mainProtoView]);
541542
compiler.compileInHost(MainComponent)
@@ -694,9 +695,9 @@ class FakeProtoViewFactory extends ProtoViewFactory {
694695
}
695696

696697
createAppProtoViews(componentBinding: DirectiveBinding, renderProtoView: renderApi.ProtoViewDto,
697-
directives: List<DirectiveBinding>): AppProtoView {
698+
directives: List<DirectiveBinding>): AppProtoView[] {
698699
this.requests.push([componentBinding, renderProtoView, directives]);
699-
return ListWrapper.removeAt(this.results, 0);
700+
return collectEmbeddedPvs(ListWrapper.removeAt(this.results, 0));
700701
}
701702
}
702703

@@ -706,4 +707,17 @@ class MergedRenderProtoViewRef extends renderApi.RenderProtoViewRef {
706707

707708
function originalRenderProtoViewRefs(appProtoView: AppProtoView) {
708709
return (<MergedRenderProtoViewRef>appProtoView.mergeMapping.renderProtoViewRef).originals;
710+
}
711+
712+
function collectEmbeddedPvs(pv: AppProtoView, target: AppProtoView[] = null): AppProtoView[] {
713+
if (isBlank(target)) {
714+
target = [];
715+
}
716+
target.push(pv);
717+
pv.elementBinders.forEach(elementBinder => {
718+
if (elementBinder.hasEmbeddedProtoView()) {
719+
collectEmbeddedPvs(elementBinder.nestedProtoView, target);
720+
}
721+
});
722+
return target;
709723
}

‎modules/angular2/test/core/compiler/projection_integration_spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -486,4 +486,4 @@ class Tab {
486486
})
487487
class Tree {
488488
depth = 0;
489-
}
489+
}

‎modules/angular2/test/core/compiler/proto_view_factory_spec.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,10 +66,10 @@ export function main() {
6666
var varBindings = new Map();
6767
varBindings.set('a', 'b');
6868
var renderPv = createRenderProtoView([], null, varBindings);
69-
var appPv =
69+
var appPvs =
7070
protoViewFactory.createAppProtoViews(bindDirective(MainComponent), renderPv, []);
71-
expect(appPv.variableBindings.get('a')).toEqual('b');
72-
expect(appPv).toBeTruthy();
71+
expect(appPvs[0].variableBindings.get('a')).toEqual('b');
72+
expect(appPvs.length).toBe(1);
7373
});
7474
});
7575

0 commit comments

Comments
 (0)
X Tutup