Skip to content

Commit 0b0080b

Browse files
rkirovjbdeboer
rkirov
authored and
jbdeboer
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. Closes dart-archive#1364
1 parent 0720c3c commit 0b0080b

File tree

2 files changed

+99
-28
lines changed

2 files changed

+99
-28
lines changed

‎lib/core_dom/directive_injector.dart

+72-26
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,14 @@ final SOURCE_LIGHT_DOM_KEY = new Key(SourceLightDom);
2626
final TEMPLATE_LOADER_KEY = new Key(TemplateLoader);
2727
final SHADOW_ROOT_KEY = new Key(ShadowRoot);
2828

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

3138
const int VISIBILITY_LOCAL = -1;
3239
const int VISIBILITY_DIRECT_CHILD = -2;
@@ -132,6 +139,12 @@ class DirectiveInjector implements DirectiveBinder {
132139
Key _key8 = null; dynamic _obj8; List<Key> _pKeys8; Function _factory8;
133140
Key _key9 = null; dynamic _obj9; List<Key> _pKeys9; Function _factory9;
134141

142+
// Keeps track of how many dependent constructions are being performed
143+
// in the directive injector.
144+
// It should be NO_CONSTRUCTION, until first call into _new sets it to 1
145+
// incremented on after each next call.
146+
int _constructionDepth;
147+
135148
@Deprecated("Deprecated. Use getFromParent instead.")
136149
Object get parent => this._parent;
137150

@@ -153,16 +166,18 @@ class DirectiveInjector implements DirectiveBinder {
153166
DirectiveInjector(DirectiveInjector parent, appInjector, this._node, this._nodeAttrs,
154167
this._eventHandler, this.scope, this._animate, [View view])
155168
: _parent = parent,
156-
_appInjector = appInjector,
157-
_view = view == null && parent != null ? parent._view : view;
169+
_appInjector = appInjector,
170+
_view = view == null && parent != null ? parent._view : view,
171+
_constructionDepth = NO_CONSTRUCTION;
158172

159173
DirectiveInjector._default(this._parent, this._appInjector)
160174
: _node = null,
161175
_nodeAttrs = null,
162176
_eventHandler = null,
163177
scope = null,
164178
_view = null,
165-
_animate = null;
179+
_animate = null,
180+
_constructionDepth = NO_CONSTRUCTION;
166181

167182
void bind(key, {dynamic toValue: DEFAULT_VALUE,
168183
Function toFactory: DEFAULT_VALUE,
@@ -213,9 +228,6 @@ class DirectiveInjector implements DirectiveBinder {
213228
var oldTag = _TAG_GET.makeCurrent();
214229
try {
215230
return _getByKey(key, _appInjector);
216-
} on ResolvingError catch (e, s) {
217-
e.appendKey(key);
218-
rethrow;
219231
} finally {
220232
oldTag.makeCurrent();
221233
}
@@ -230,42 +242,47 @@ class DirectiveInjector implements DirectiveBinder {
230242
}
231243

232244
Object _getByKey(Key key, Injector appInjector) {
233-
int uid = key.uid;
234-
if (uid == null || uid == UNDEFINED_ID) return appInjector.getByKey(key);
235-
bool isDirective = uid < 0;
236-
return isDirective ? _getDirectiveByKey(key, uid, appInjector) : _getById(uid);
245+
try {
246+
int uid = key.uid;
247+
if (uid == null || uid == UNDEFINED_ID) return appInjector.getByKey(key);
248+
bool isDirective = uid < 0;
249+
return isDirective ? _getDirectiveByKey(key, uid, appInjector) : _getById(uid);
250+
} on ResolvingError catch (e) {
251+
e.appendKey(key);
252+
rethrow;
253+
}
237254
}
238255

239256
num _getDepth(int visType) {
240257
switch(visType) {
241258
case VISIBILITY_LOCAL: return 0;
242259
case VISIBILITY_DIRECT_CHILD: return 1;
243-
case VISIBILITY_CHILDREN: return MAX_DEPTH;
260+
case VISIBILITY_CHILDREN: return MAX_TREE_DEPTH;
244261
default: throw null;
245262
}
246263
}
247264

248265
_getDirectiveByKey(Key k, int visType, Injector appInjector) {
249-
num depth = _getDepth(visType);
266+
num treeDepth = _getDepth(visType);
250267
// ci stands for currentInjector, abbreviated for readability.
251268
var ci = this;
252-
while (ci != null && depth >= 0) {
269+
while (ci != null && treeDepth >= 0) {
253270
do {
254-
if (ci._key0 == null) break; if (identical(ci._key0, k)) return ci._obj0 == null ? ci._obj0 = ci._new(ci._pKeys0, ci._factory0) : ci._obj0;
255-
if (ci._key1 == null) break; if (identical(ci._key1, k)) return ci._obj1 == null ? ci._obj1 = ci._new(ci._pKeys1, ci._factory1) : ci._obj1;
256-
if (ci._key2 == null) break; if (identical(ci._key2, k)) return ci._obj2 == null ? ci._obj2 = ci._new(ci._pKeys2, ci._factory2) : ci._obj2;
257-
if (ci._key3 == null) break; if (identical(ci._key3, k)) return ci._obj3 == null ? ci._obj3 = ci._new(ci._pKeys3, ci._factory3) : ci._obj3;
258-
if (ci._key4 == null) break; if (identical(ci._key4, k)) return ci._obj4 == null ? ci._obj4 = ci._new(ci._pKeys4, ci._factory4) : ci._obj4;
259-
if (ci._key5 == null) break; if (identical(ci._key5, k)) return ci._obj5 == null ? ci._obj5 = ci._new(ci._pKeys5, ci._factory5) : ci._obj5;
260-
if (ci._key6 == null) break; if (identical(ci._key6, k)) return ci._obj6 == null ? ci._obj6 = ci._new(ci._pKeys6, ci._factory6) : ci._obj6;
261-
if (ci._key7 == null) break; if (identical(ci._key7, k)) return ci._obj7 == null ? ci._obj7 = ci._new(ci._pKeys7, ci._factory7) : ci._obj7;
262-
if (ci._key8 == null) break; if (identical(ci._key8, k)) return ci._obj8 == null ? ci._obj8 = ci._new(ci._pKeys8, ci._factory8) : ci._obj8;
263-
if (ci._key9 == null) break; if (identical(ci._key9, k)) return ci._obj9 == null ? ci._obj9 = ci._new(ci._pKeys9, ci._factory9) : ci._obj9;
271+
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;
272+
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;
273+
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;
274+
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;
275+
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;
276+
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;
277+
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;
278+
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;
279+
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;
280+
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;
264281
} while (false);
265282
// Future feature: Component Injectors fall-through only if directly called.
266283
// if ((ci is ComponentDirectiveInjector) && !identical(ci, this)) break;
267284
ci = ci._parent;
268-
depth--;
285+
treeDepth--;
269286
}
270287
return appInjector.getByKey(k);
271288
}
@@ -304,7 +321,12 @@ class DirectiveInjector implements DirectiveBinder {
304321
}
305322
}
306323

307-
dynamic _new(List<Key> paramKeys, Function fn) {
324+
dynamic _new(Key k, List<Key> paramKeys, Function fn) {
325+
if (_constructionDepth > MAX_CONSTRUCTION_DEPTH) {
326+
_constructionDepth = NO_CONSTRUCTION;
327+
throw new DiCircularDependencyError(key);
328+
}
329+
bool isFirstConstruction = (_constructionDepth++ == NO_CONSTRUCTION);
308330
var oldTag = _TAG_GET.makeCurrent();
309331
int size = paramKeys.length;
310332
var obj;
@@ -353,6 +375,7 @@ class DirectiveInjector implements DirectiveBinder {
353375
}
354376
}
355377
oldTag.makeCurrent();
378+
if (isFirstConstruction) _constructionDepth = NO_CONSTRUCTION;
356379
return obj;
357380
}
358381

@@ -451,3 +474,26 @@ class ComponentDirectiveInjector extends DirectiveInjector {
451474
// For example, a local directive is visible from its component injector children.
452475
num _getDepth(int visType) => super._getDepth(visType) + 1;
453476
}
477+
478+
479+
// For efficiency we run through the maximum resolving depth and unwind
480+
// instead of setting 'resolving' key per type.
481+
class DiCircularDependencyError extends ResolvingError {
482+
DiCircularDependencyError(key) : super(key);
483+
484+
// strips the cyclical part of the chain.
485+
List<Key> get stripCycle {
486+
List<Key> rkeys = keys.reversed.toList();
487+
for (num i = 0; i < rkeys.length; i++) {
488+
// Skipping 'cycles' of length 1, which represent resolution
489+
// switching injectors and not true cycles.
490+
for (num j = i + 2; j < rkeys.length; j++) {
491+
if (rkeys[i] == rkeys[j]) return rkeys.sublist(0, j + 1);
492+
}
493+
}
494+
return rkeys;
495+
}
496+
497+
String get resolveChain => stripCycle.join(' -> ');
498+
String toString() => "circular dependency (${resolveChain})";
499+
}

‎test/core_dom/directive_injector_spec.dart

+27-2
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,11 @@ void main() {
1818
View view;
1919
Animate animate;
2020

21-
addDirective(Type type, [Visibility visibility]) {
21+
addDirective(Type type, [Visibility visibility, DirectiveInjector targetInjector]) {
22+
if (targetInjector == null) targetInjector = injector;
2223
if (visibility == null) visibility = Visibility.LOCAL;
2324
var reflector = Module.DEFAULT_REFLECTOR;
24-
injector.bindByKey(
25+
targetInjector.bindByKey(
2526
new Key(type),
2627
reflector.factoryFor(type),
2728
reflector.parameterKeysFor(type),
@@ -107,6 +108,27 @@ void main() {
107108
});
108109
});
109110

111+
describe('error handling', () {
112+
it('should throw circular dependency error', () {
113+
addDirective(_TypeC0);
114+
addDirective(_TypeC1, Visibility.CHILDREN);
115+
addDirective(_TypeC2, Visibility.CHILDREN);
116+
expect(() => injector.get(_TypeC0)).toThrow(
117+
'circular dependency (_TypeC0 -> _TypeC1 -> _TypeC2 -> _TypeC1)');
118+
});
119+
120+
it('should throw circular dependency error accross injectors', () {
121+
var childInjector =
122+
new DirectiveInjector(injector, appInjector, null, null, null, null, null);
123+
124+
addDirective(_TypeC0, Visibility.LOCAL, childInjector);
125+
addDirective(_TypeC1, Visibility.CHILDREN);
126+
addDirective(_TypeC2, Visibility.CHILDREN);
127+
expect(() => childInjector.get(_TypeC0)).toThrow(
128+
'circular dependency (_TypeC0 -> _TypeC1 -> _TypeC2 -> _TypeC1)');
129+
});
130+
});
131+
110132
describe('Visibility', () {
111133
DirectiveInjector childInjector;
112134
DirectiveInjector leafInjector;
@@ -161,3 +183,6 @@ class _Type8{ final _Type7 type7; _Type8(this.type7); }
161183
class _Type9{ final _Type8 type8; _Type9(this.type8); }
162184
class _TypeA{ final _Type9 type9; _TypeA(this.type9); }
163185

186+
class _TypeC0 {final _TypeC1 t; _TypeC0(this.t);}
187+
class _TypeC1 {final _TypeC2 t; _TypeC1(this.t);}
188+
class _TypeC2 {final _TypeC1 t; _TypeC2(this.t);}

0 commit comments

Comments
 (0)