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

Mem leak #700

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions lib/change_detection/watch_group.dart
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,7 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {

// Unlink the _watchRecord
_EvalWatchRecord firstEvalWatch = _evalWatchHead;
_EvalWatchRecord lastEvalWatch =
(_watchGroupTail == null ? this : _watchGroupTail)._evalWatchTail;
_EvalWatchRecord lastEvalWatch = _childWatchGroupTail._evalWatchTail;
_EvalWatchRecord previous = firstEvalWatch._prevEvalWatch;
_EvalWatchRecord next = lastEvalWatch._nextEvalWatch;
if (previous != null) previous._nextEvalWatch = next;
Expand Down
37 changes: 33 additions & 4 deletions lib/core/scope.dart
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ class ScopeLocals implements Map {
* for change detection, change processing and memory management.
*/
class Scope {
final String id;
int _childScopeNextId = 0;

/**
* The default execution context for [watch]es [observe]ers, and [eval]uation.
Expand Down Expand Up @@ -182,7 +184,7 @@ class Scope {
bool get hasOwnStreams => _streams != null && _streams._scope == this;

Scope(Object this.context, this.rootScope, this._parentScope,
this._readWriteGroup, this._readOnlyGroup);
this._readWriteGroup, this._readOnlyGroup, this.id);

/**
* A [watch] sets up a watch in the [digest] phase of the [apply] cycle.
Expand Down Expand Up @@ -271,7 +273,8 @@ class Scope {
assert(isAttached);
var child = new Scope(childContext, rootScope, this,
_readWriteGroup.newGroup(childContext),
_readOnlyGroup.newGroup(childContext));
_readOnlyGroup.newGroup(childContext),
'$id:${_childScopeNextId++}');

var prev = _childTail;
child._prev = prev;
Expand Down Expand Up @@ -301,6 +304,26 @@ class Scope {
_readWriteGroup.remove();
_readOnlyGroup.remove();
_parentScope = null;

assert((() {
var scopes = [this];
while(scopes.isNotEmpty) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space here and below

Scope scope = scopes.removeAt(0);
Scope childScope = scope._childHead;
while(childScope != null) {
scopes.add(childScope);
childScope = childScope._next;
}

scope._next = scope._prev = null;
scope._childHead = scope._childTail = null;
if (scope._streams != null) scope._streams._release();
scope._streams = null;
}

return true;
})());

}

_assertInternalStateConsistency() {
Expand Down Expand Up @@ -422,7 +445,8 @@ class RootScope extends Scope {
this._scopeStats)
: super(context, null, null,
new RootWatchGroup(new DirtyCheckingChangeDetector(cacheGetter), context),
new RootWatchGroup(new DirtyCheckingChangeDetector(cacheGetter), context))
new RootWatchGroup(new DirtyCheckingChangeDetector(cacheGetter), context),
'')
{
_zone.onTurnDone = apply;
_zone.onError = (e, s, ls) => _exceptionHandler(e, s);
Expand Down Expand Up @@ -595,7 +619,7 @@ class RootScope extends Scope {
class _Streams {
final ExceptionHandler _exceptionHandler;
/// Scope we belong to.
final Scope _scope;
/*final*/ Scope _scope;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the commen?

/// [Stream]s for [_scope] only
final _streams = new Map<String, ScopeStream>();
/// Child [Scope] event counts.
Expand Down Expand Up @@ -729,6 +753,11 @@ class _Streams {
scope = scope._parentScope;
}
}

void _release() {
_scope = null;
_streams.clear();
}
}

class ScopeStream extends async.Stream<ScopeEvent> {
Expand Down
11 changes: 7 additions & 4 deletions test/change_detection/watch_group_spec.dart
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ void main() {
expect(watchGrp.totalFieldCost).toEqual(2);
});

it('should remove all method watches in group and group\'s children', () {
iit('should remove all method watches in group and group\'s children', () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iit

context['my'] = new MyClass(logger);
AST countMethod = new MethodAST(parse('my'), 'count', []);
watchGrp.watch(countMethod, (v, p) => logger('0a'));
Expand All @@ -602,6 +602,7 @@ void main() {
var child1a = watchGrp.newGroup(new PrototypeMap(context));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on another note, this spec is really long, does it need to be so verbose? (this point was made about the js port as well, it might be good to simplify these so that they're easier to follow and more clear about what they're testing)

var child1b = watchGrp.newGroup(new PrototypeMap(context));
var child2 = child1a.newGroup(new PrototypeMap(context));
var child3 = child2.newGroup(new PrototypeMap(context));
child1a.watch(countMethod, (v, p) => logger('1a'));
expectOrder(['0a', '1a']);
child1b.watch(countMethod, (v, p) => logger('1b'));
Expand All @@ -612,12 +613,14 @@ void main() {
expectOrder(['0a', '0A', '1a', '1A', '1b']);
child2.watch(countMethod, (v, p) => logger('2A'));
expectOrder(['0a', '0A', '1a', '1A', '2A', '1b']);
child3.watch(countMethod, (v, p) => logger('3'));
expectOrder(['0a', '0A', '1a', '1A', '2A', '3', '1b']);

// flush initial reaction functions
expect(watchGrp.detectChanges()).toEqual(6);
expectOrder(['0a', '0A', '1a', '1A', '2A', '1b']);
expect(watchGrp.detectChanges()).toEqual(7);
expectOrder(['0a', '0A', '1a', '1A', '2A', '3', '1b']);

child1a.remove(); // should also remove child2
child1a.remove(); // should also remove child2 and child 3
expect(watchGrp.detectChanges()).toEqual(3);
expectOrder(['0a', '0A', '1b']);
});
Expand Down
2 changes: 2 additions & 0 deletions test/core/scope_spec.dart
Original file line number Diff line number Diff line change
Expand Up @@ -224,11 +224,13 @@ void main() {
describe('parent', () {
it('should not have parent', inject((RootScope rootScope) {
expect(rootScope.parentScope).toEqual(null);
expect(rootScope.id).toEqual('');
}));


it('should point to parent', inject((RootScope rootScope) {
var child = rootScope.createChild(new PrototypeMap(rootScope.context));
expect(child.id).toEqual(':0');
expect(rootScope.parentScope).toEqual(null);
expect(child.parentScope).toEqual(rootScope);
expect(child.createChild(new PrototypeMap(rootScope.context)).parentScope).toEqual(child);
Expand Down