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

Commit

Permalink
fix(dirty-checking): fix removal of watch record on disconnected group.
Browse files Browse the repository at this point in the history
  • Loading branch information
@mhevery
mhevery committed Mar 25, 2014
1 parent a0d820c commit d22899a
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 16 deletions.
61 changes: 46 additions & 15 deletions lib/change_detection/dirty_checking_change_detector.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,23 @@ class DirtyCheckingChangeDetectorGroup<H> implements ChangeDetectorGroup<H> {
return tail._recordTail;
}

bool get isAttached {
DirtyCheckingChangeDetectorGroup current = this;
DirtyCheckingChangeDetectorGroup parent;
while ((parent = current._parent) != null) {
current = parent;
}
return current is DirtyCheckingChangeDetector
? true
: current._prev != null && current._next != null;
}


DirtyCheckingChangeDetector get _root {
var root = this;
var next;
while ((next = root._parent) != null) {
root = next;
var parent;
while ((parent = root._parent) != null) {
root = parent;
}
return root is DirtyCheckingChangeDetector ? root : null;
}
Expand Down Expand Up @@ -146,16 +157,12 @@ class DirtyCheckingChangeDetectorGroup<H> implements ChangeDetectorGroup<H> {
assert((root = _root) != null);
assert(root._assertRecordsOk());
DirtyCheckingRecord prevRecord = _recordHead._prevRecord;
DirtyCheckingRecord nextRecord = _childInclRecordTail._nextRecord;
var childInclRecordTail = _childInclRecordTail;
DirtyCheckingRecord nextRecord = childInclRecordTail._nextRecord;

if (prevRecord != null) prevRecord._nextRecord = nextRecord;
if (nextRecord != null) nextRecord._prevRecord = prevRecord;

var cursor = _recordHead;
while (cursor != nextRecord) {
cursor = cursor._nextRecord;
}

var prevGroup = _prev;
var nextGroup = _next;

Expand All @@ -172,8 +179,7 @@ class DirtyCheckingChangeDetectorGroup<H> implements ChangeDetectorGroup<H> {
_parent = null;
_prev = _next = null;
_recordHead._prevRecord = null;
_recordTail._nextRecord = null;
_recordHead = _recordTail = null;
childInclRecordTail._nextRecord = null;

This comment has been minimized.

Sign in to view

Copy link
@mhevery

mhevery Mar 25, 2014

Author Contributor

@caitp This line probably needs to be fixed in watchtower.js
assert(root._assertRecordsOk());
}

Expand Down Expand Up @@ -222,8 +228,8 @@ class DirtyCheckingChangeDetectorGroup<H> implements ChangeDetectorGroup<H> {
do {
allRecords.add(record.toString());
record = record._nextRecord;
}
while (record != includeChildrenTail);
} while (record != includeChildrenTail);
allRecords.add(includeChildrenTail);
lines.add('FIELDS: ${allRecords.join(', ')}');
}

Expand Down Expand Up @@ -267,17 +273,42 @@ class DirtyCheckingChangeDetector<H> extends DirtyCheckingChangeDetectorGroup<H>
}
var groupRecord = group._recordHead;
var groupLast = group._recordTail;
while (true) {
if (record != groupRecord) {
throw "Next record is $record expecting $groupRecord";
}
var done = false;
while (!done && groupRecord != null) {
if (groupRecord == record) {
if (record._group != null && record._group != group) {
throw "Wrong group: $record "
"Got ${record._group} Expecting: $group";
}
record = record._nextRecord;
} else {
throw 'lost: $record found $groupRecord\n$this';
}

if (groupRecord == groupLast) break;
if (groupRecord._nextRecord != null &&
groupRecord._nextRecord._prevRecord != groupRecord) {
throw "prev/next pointer missmatch on "
"$groupRecord -> ${groupRecord._nextRecord} "
"<= ${groupRecord._nextRecord._prevRecord} in $this";
}
if (groupRecord._prevRecord != null &&
groupRecord._prevRecord._nextRecord != groupRecord) {
throw "prev/next pointer missmatch on "
"$groupRecord -> ${groupRecord._prevRecord} "
"<= ${groupRecord._prevRecord._nextChange} in $this";
}
if (groupRecord == groupLast) {
done = true;
}
groupRecord = groupRecord._nextRecord;
}
}
if(record != null) {
throw "Extra records at tail: $record on $this";
}
return true;
}

Expand Down
78 changes: 77 additions & 1 deletion test/change_detection/dirty_checking_change_detector_spec.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ import '../_specs.dart';
import 'package:angular/change_detection/change_detection.dart';
import 'package:angular/change_detection/dirty_checking_change_detector.dart';
import 'dart:collection';
import 'dart:math';

void main() {
describe('DirtyCheckingChangeDetector', () {
DirtyCheckingChangeDetector<String> detector;
GetterCache getterCache;

beforeEach(() {
GetterCache getterCache = new GetterCache({
getterCache = new GetterCache({
"first": (o) => o.first,
"age": (o) => o.age
});
Expand Down Expand Up @@ -170,6 +172,80 @@ void main() {
var b = detector.newGroup();
expect(detector.collectChanges).not.toThrow();
});

it('should properly disconnect group in case watch is removed in disconected group', () {
var map = {};
var detector0 = new DirtyCheckingChangeDetector<String>(getterCache);
var detector1 = detector0.newGroup();
var detector2 = detector1.newGroup();
var watch2 = detector2.watch(map, 'f1', null);
var detector3 = detector0.newGroup();
detector1.remove();
watch2.remove(); // removing a dead record
detector3.watch(map, 'f2', null);
});

it('should find random bugs', () {

This comment has been minimized.

Sign in to view

Copy link
@caitp

caitp Mar 25, 2014

Contributor

@mhevery I'll investigate this commit and the commented line to see if/how it should go into watchtower.

But this testcase looks really scary, it's kind of hard to understand what's being verified from the description, and it's very long, sort of hard to follow.

I've done some work to break up other tests into more atomic pieces, so that they're easier to follow (and if they fail, easier to understand what broke). I think it might be a good idea to do that in the dart code as well, especially for this test case.
List detectors;
List records;
List steps;
var field = 'someField';
step(text) {
//print(text);
steps.add(text);
}
Map map = {};
var random = new Random();
try {
for (var i = 0; i < 100000; i++) {
if (i % 50 == 0) {
//print(steps);
//print('===================================');
records = [];
steps = [];
detectors = [new DirtyCheckingChangeDetector<String>(getterCache)];
}
switch (random.nextInt(4)) {
case 0: // new child detector
if (detectors.length > 10) break;
var index = random.nextInt(detectors.length);
ChangeDetectorGroup detector = detectors[index];
step('detectors[$index].newGroup()');
var child = detector.newGroup();
detectors.add(child);
break;
case 1: // add watch
var index = random.nextInt(detectors.length);
ChangeDetectorGroup detector = detectors[index];
step('detectors[$index].watch(map, field, null)');
WatchRecord record = detector.watch(map, field, null);
records.add(record);
break;
case 2: // destroy watch group
if (detectors.length == 1) break;
var index = random.nextInt(detectors.length - 1) + 1;
ChangeDetectorGroup detector = detectors[index];
step('detectors[$index].remove()');
detector.remove();
detectors = detectors
.where((s) => s.isAttached)
.toList();
break;
case 3: // remove watch on watch group
if (records.length == 0) break;
var index = random.nextInt(records.length);
WatchRecord record = records.removeAt(index);
step('records.removeAt($index).remove()');
record.remove();
break;
}
}
} catch(e) {
print(steps);
rethrow;
}
});

});

describe('list watching', () {
Expand Down

0 comments on commit d22899a

Please sign in to comment.