Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

Commit 995280b

Browse files
rkirovrkirov
committed
feat(directive-injector): detect and throw on circular deps
The maximum depth for depth for dependency construction within a single injector is set to 50. No performance degradation observed on tree benchmark.
1 parent 1eb6d2a commit 995280b

File tree

2 files changed

+99
-27
lines changed

2 files changed

+99
-27
lines changed

‎lib/core_dom/directive_injector.dart

+71-25
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,14 @@ final CONTENT_PORT_KEY = new Key(ContentPort);
2424
final TEMPLATE_LOADER_KEY = new Key(TemplateLoader);
2525
final SHADOW_ROOT_KEY = new Key(ShadowRoot);
2626

27-
final num MAX_DEPTH = 1 << 30;
27+
final int NO_CONSTRUCTION = 0;
28+
29+
// Maximum parent directive injectors that would be traversed.
30+
final int MAX_TREE_DEPTH = 1 << 30;
31+
32+
// Maximum number of concurrent constructions a directive injector would
33+
// support before throwing an error.
34+
final int MAX_CONSTRUCTION_DEPTH = 50;
2835

2936
const int VISIBILITY_LOCAL = -1;
3037
const int VISIBILITY_DIRECT_CHILD = -2;
@@ -122,6 +129,12 @@ class DirectiveInjector implements DirectiveBinder {
122129
Key _key8 = null; dynamic _obj8; List<Key> _pKeys8; Function _factory8;
123130
Key _key9 = null; dynamic _obj9; List<Key> _pKeys9; Function _factory9;
124131

132+
// Keeps track of how many dependent constructions are being performed
133+
// in the directive injector.
134+
// It should be NO_CONSTRUCTION, until first call into _new sets it to 1
135+
// incremented on after each next call.
136+
int _constructionDepth;
137+
125138
@Deprecated("Deprecated. Use getFromParent instead.")
126139
Object get parent => this._parent;
127140

@@ -142,14 +155,17 @@ class DirectiveInjector implements DirectiveBinder {
142155

143156
DirectiveInjector(this._parent, appInjector, this._node, this._nodeAttrs,
144157
this._eventHandler, this.scope, this._animate)
145-
: _appInjector = appInjector;
158+
: _appInjector = appInjector,
159+
_constructionDepth = NO_CONSTRUCTION;
160+
146161

147162
DirectiveInjector._default(this._parent, this._appInjector)
148163
: _node = null,
149164
_nodeAttrs = null,
150165
_eventHandler = null,
151166
scope = null,
152-
_animate = null;
167+
_animate = null,
168+
_constructionDepth = NO_CONSTRUCTION;
153169

154170
void bind(key, {dynamic toValue: DEFAULT_VALUE,
155171
Function toFactory: DEFAULT_VALUE,
@@ -200,9 +216,6 @@ class DirectiveInjector implements DirectiveBinder {
200216
var oldTag = _TAG_GET.makeCurrent();
201217
try {
202218
return _getByKey(key, _appInjector);
203-
} on ResolvingError catch (e, s) {
204-
e.appendKey(key);
205-
rethrow;
206219
} finally {
207220
oldTag.makeCurrent();
208221
}
@@ -217,42 +230,47 @@ class DirectiveInjector implements DirectiveBinder {
217230
}
218231

219232
Object _getByKey(Key key, Injector appInjector) {
220-
int uid = key.uid;
221-
if (uid == null || uid == UNDEFINED_ID) return appInjector.getByKey(key);
222-
bool isDirective = uid < 0;
223-
return isDirective ? _getDirectiveByKey(key, uid, appInjector) : _getById(uid);
233+
try {
234+
int uid = key.uid;
235+
if (uid == null || uid == UNDEFINED_ID) return appInjector.getByKey(key);
236+
bool isDirective = uid < 0;
237+
return isDirective ? _getDirectiveByKey(key, uid, appInjector) : _getById(uid);
238+
} on ResolvingError catch (e) {
239+
e.appendKey(key);
240+
rethrow;
241+
}
224242
}
225243

226244
num _getDepth(int visType) {
227245
switch(visType) {
228246
case VISIBILITY_LOCAL: return 0;
229247
case VISIBILITY_DIRECT_CHILD: return 1;
230-
case VISIBILITY_CHILDREN: return MAX_DEPTH;
248+
case VISIBILITY_CHILDREN: return MAX_TREE_DEPTH;
231249
default: throw null;
232250
}
233251
}
234252

235253
_getDirectiveByKey(Key k, int visType, Injector appInjector) {
236-
num depth = _getDepth(visType);
254+
num treeDepth = _getDepth(visType);
237255
// ci stands for currentInjector, abbreviated for readability.
238256
var ci = this;
239-
while (ci != null && depth >= 0) {
257+
while (ci != null && treeDepth >= 0) {
240258
do {
241-
if (ci._key0 == null) break; if (identical(ci._key0, k)) return ci._obj0 == null ? ci._obj0 = ci._new(ci._pKeys0, ci._factory0) : ci._obj0;
242-
if (ci._key1 == null) break; if (identical(ci._key1, k)) return ci._obj1 == null ? ci._obj1 = ci._new(ci._pKeys1, ci._factory1) : ci._obj1;
243-
if (ci._key2 == null) break; if (identical(ci._key2, k)) return ci._obj2 == null ? ci._obj2 = ci._new(ci._pKeys2, ci._factory2) : ci._obj2;
244-
if (ci._key3 == null) break; if (identical(ci._key3, k)) return ci._obj3 == null ? ci._obj3 = ci._new(ci._pKeys3, ci._factory3) : ci._obj3;
245-
if (ci._key4 == null) break; if (identical(ci._key4, k)) return ci._obj4 == null ? ci._obj4 = ci._new(ci._pKeys4, ci._factory4) : ci._obj4;
246-
if (ci._key5 == null) break; if (identical(ci._key5, k)) return ci._obj5 == null ? ci._obj5 = ci._new(ci._pKeys5, ci._factory5) : ci._obj5;
247-
if (ci._key6 == null) break; if (identical(ci._key6, k)) return ci._obj6 == null ? ci._obj6 = ci._new(ci._pKeys6, ci._factory6) : ci._obj6;
248-
if (ci._key7 == null) break; if (identical(ci._key7, k)) return ci._obj7 == null ? ci._obj7 = ci._new(ci._pKeys7, ci._factory7) : ci._obj7;
249-
if (ci._key8 == null) break; if (identical(ci._key8, k)) return ci._obj8 == null ? ci._obj8 = ci._new(ci._pKeys8, ci._factory8) : ci._obj8;
250-
if (ci._key9 == null) break; if (identical(ci._key9, k)) return ci._obj9 == null ? ci._obj9 = ci._new(ci._pKeys9, ci._factory9) : ci._obj9;
259+
if (ci._key0 == null) break; if (identical(ci._key0, k)) return ci._obj0 == null ? ci._obj0 = ci._new(k, ci._pKeys0, ci._factory0) : ci._obj0;
260+
if (ci._key1 == null) break; if (identical(ci._key1, k)) return ci._obj1 == null ? ci._obj1 = ci._new(k, ci._pKeys1, ci._factory1) : ci._obj1;
261+
if (ci._key2 == null) break; if (identical(ci._key2, k)) return ci._obj2 == null ? ci._obj2 = ci._new(k, ci._pKeys2, ci._factory2) : ci._obj2;
262+
if (ci._key3 == null) break; if (identical(ci._key3, k)) return ci._obj3 == null ? ci._obj3 = ci._new(k, ci._pKeys3, ci._factory3) : ci._obj3;
263+
if (ci._key4 == null) break; if (identical(ci._key4, k)) return ci._obj4 == null ? ci._obj4 = ci._new(k, ci._pKeys4, ci._factory4) : ci._obj4;
264+
if (ci._key5 == null) break; if (identical(ci._key5, k)) return ci._obj5 == null ? ci._obj5 = ci._new(k, ci._pKeys5, ci._factory5) : ci._obj5;
265+
if (ci._key6 == null) break; if (identical(ci._key6, k)) return ci._obj6 == null ? ci._obj6 = ci._new(k, ci._pKeys6, ci._factory6) : ci._obj6;
266+
if (ci._key7 == null) break; if (identical(ci._key7, k)) return ci._obj7 == null ? ci._obj7 = ci._new(k, ci._pKeys7, ci._factory7) : ci._obj7;
267+
if (ci._key8 == null) break; if (identical(ci._key8, k)) return ci._obj8 == null ? ci._obj8 = ci._new(k, ci._pKeys8, ci._factory8) : ci._obj8;
268+
if (ci._key9 == null) break; if (identical(ci._key9, k)) return ci._obj9 == null ? ci._obj9 = ci._new(k, ci._pKeys9, ci._factory9) : ci._obj9;
251269
} while (false);
252270
// Future feature: Component Injectors fall-through only if directly called.
253271
// if ((ci is ComponentDirectiveInjector) && !identical(ci, this)) break;
254272
ci = ci._parent;
255-
depth--;
273+
treeDepth--;
256274
}
257275
return appInjector.getByKey(k);
258276
}
@@ -295,7 +313,12 @@ class DirectiveInjector implements DirectiveBinder {
295313
}
296314
}
297315

298-
dynamic _new(List<Key> paramKeys, Function fn) {
316+
dynamic _new(Key k, List<Key> paramKeys, Function fn) {
317+
if (_constructionDepth > MAX_CONSTRUCTION_DEPTH) {
318+
_constructionDepth = NO_CONSTRUCTION;
319+
throw new DiCircularDependencyError(key);
320+
}
321+
bool isFirstConstruction = (_constructionDepth++ == NO_CONSTRUCTION);
299322
var oldTag = _TAG_GET.makeCurrent();
300323
int size = paramKeys.length;
301324
var obj;
@@ -344,6 +367,7 @@ class DirectiveInjector implements DirectiveBinder {
344367
}
345368
}
346369
oldTag.makeCurrent();
370+
if (isFirstConstruction) _constructionDepth = NO_CONSTRUCTION;
347371
return obj;
348372
}
349373

@@ -421,3 +445,25 @@ class ComponentDirectiveInjector extends DirectiveInjector {
421445
num _getDepth(int visType) => super._getDepth(visType) + 1;
422446
}
423447

448+
449+
// For efficiency we run through the maximum resolving depth and unwind
450+
// instead of setting 'resolving' key per type.
451+
class DiCircularDependencyError extends ResolvingError {
452+
DiCircularDependencyError(key) : super(key);
453+
454+
// strips the cyclical part of the chain.
455+
List<Key> get stripCycle {
456+
List<Key> rkeys = keys.reversed.toList();
457+
for (num i = 0; i < rkeys.length; i++) {
458+
// Skipping 'cycles' of length 1, which represent resolution
459+
// switching injectors and not true cycles.
460+
for (num j = i + 2; j < rkeys.length; j++) {
461+
if (rkeys[i] == rkeys[j]) return rkeys.sublist(0, j + 1);
462+
}
463+
}
464+
return rkeys;
465+
}
466+
467+
String get resolveChain => stripCycle.join(' -> ');
468+
String toString() => "circular dependency (${resolveChain})";
469+
}

‎test/core_dom/directive_injector_spec.dart

+28-2
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,11 @@ void main() {
1717
Scope scope;
1818
Animate animate;
1919

20-
addDirective(Type type, [Visibility visibility]) {
20+
addDirective(Type type, [Visibility visibility, DirectiveInjector targetInjector]) {
21+
if (targetInjector == null) targetInjector = injector;
2122
if (visibility == null) visibility = Visibility.LOCAL;
2223
var reflector = Module.DEFAULT_REFLECTOR;
23-
injector.bindByKey(
24+
targetInjector.bindByKey(
2425
new Key(type),
2526
reflector.factoryFor(type),
2627
reflector.parameterKeysFor(type),
@@ -74,6 +75,28 @@ void main() {
7475
expect(() => injector.get(_TypeA)).toThrow('No provider found for _TypeA');
7576
});
7677

78+
79+
describe('error handling', () {
80+
it('should throw circular dependency error', () {
81+
addDirective(_TypeC0);
82+
addDirective(_TypeC1, Visibility.CHILDREN);
83+
addDirective(_TypeC2, Visibility.CHILDREN);
84+
expect(() => injector.get(_TypeC0)).toThrow(
85+
'circular dependency (_TypeC0 -> _TypeC1 -> _TypeC2 -> _TypeC1)');
86+
});
87+
88+
it('should throw circular dependency error accross injectors', () {
89+
var childInjector =
90+
new DirectiveInjector(injector, appInjector, null, null, null, null, null);
91+
92+
addDirective(_TypeC0, Visibility.LOCAL, childInjector);
93+
addDirective(_TypeC1, Visibility.CHILDREN);
94+
addDirective(_TypeC2, Visibility.CHILDREN);
95+
expect(() => childInjector.get(_TypeC0)).toThrow(
96+
'circular dependency (_TypeC0 -> _TypeC1 -> _TypeC2 -> _TypeC1)');
97+
});
98+
});
99+
77100
describe('Visibility', () {
78101
DirectiveInjector childInjector;
79102
DirectiveInjector leafInjector;
@@ -128,3 +151,6 @@ class _Type8{ final _Type7 type7; _Type8(this.type7); }
128151
class _Type9{ final _Type8 type8; _Type9(this.type8); }
129152
class _TypeA{ final _Type9 type9; _TypeA(this.type9); }
130153

154+
class _TypeC0 {final _TypeC1 t; _TypeC0(this.t);}
155+
class _TypeC1 {final _TypeC2 t; _TypeC1(this.t);}
156+
class _TypeC2 {final _TypeC1 t; _TypeC2(this.t);}

0 commit comments

Comments
 (0)