DynamoDS/Dynamo — pull request #3059 by Benglin (2014-11-13 07:11:45+00:00)
Title: Fixed evaluation failures in successive file open

Background
-----
This pull request fixes a few defects around having ```ProtoCore.DSASM.StackValue``` shown on node previews (defect details below). With most things now scheduled through ```DynamoScheduler```, the sequence of events that finally result in the problem can now be taken apart and analysed.

The following sections outline the sequence of events for different scenarios, **please keep these in mind** while reading into these sections:

1. ```DelegateBasedAsyncTask``` carries an ```Action``` to invoke ```ResetEngineInternal```, when processed, these ```DelegateBasedAsyncTask``` destroys the ```EngineController``` and make a new one.
2. ```UpdateGraphAsyncTask``` has a higher priority than ```DelegateBasedAsyncTask``` by design, if both these tasks are in the queue, ```UpdateGraphAsyncTask``` will be moved in front of ```DelegateBasedAsyncTask``` when ```DynamoScheduler``` comes around to compact the task queue.

### Working scenario
The following scenario outlines the sequence of events that take place when things work. Notice how ```DelegateBasedAsyncTask``` (which is ```ResetEngineInternal```) gets processed before ```UpdateGraphAsyncTask```. Since the graph is updated *after* a new ```EngineController``` is created, it happens on the new ```EngineController``` and subsequent ```UpdateRenderPackageAsyncTask``` successfully retrieve a node's value.

```
Schedule Task: DelegateBasedAsyncTask
Schedule Task: AggregateRenderPackageAsyncTask
Process Task: DelegateBasedAsyncTask ==> DelegateBasedAsyncTask gets processed
Schedule Task: UpdateGraphAsyncTask
Process Task: UpdateGraphAsyncTask ==> UpdateGraphAsyncTask gets processed
Schedule Task: UpdateRenderPackageAsyncTask
Schedule Task: NotifyRenderPackagesReadyAsyncTask
Process Task: AggregateRenderPackageAsyncTask
Process Task: UpdateRenderPackageAsyncTask ==> UpdateRenderPackageAsyncTask gets correct value
Process Task: NotifyRenderPackagesReadyAsyncTask
Schedule Task: AggregateRenderPackageAsyncTask
Process Task: AggregateRenderPackageAsyncTask
```

### Failure scenario
For scenario that fails, the following sequence of events took place. Since ```DelegateBasedAsyncTask``` destroys the ```EngineController``` after ```UpdateGraphAsyncTask``` is processed, it destroys everything that was constructed during ```UpdateGraphAsyncTask```. This causes ```UpdateRenderPackageAsyncTask``` to get an invalid or ```null``` value at a later time.

```
Schedule Task: DelegateBasedAsyncTask
Schedule Task: AggregateRenderPackageAsyncTask
Schedule Task: UpdateGraphAsyncTask
Process Task: UpdateGraphAsyncTask ==> UpdateGraphAsyncTask is of higher priority
Schedule Task: UpdateRenderPackageAsyncTask
Schedule Task: NotifyRenderPackagesReadyAsyncTask
Process Task: DelegateBasedAsyncTask ==> DelegateBasedAsyncTask destroys EngineController
Process Task: AggregateRenderPackageAsyncTask
Process Task: UpdateRenderPackageAsyncTask ==> UpdateRenderPackageAsyncTask gets invalid value
Process Task: NotifyRenderPackagesReadyAsyncTask
Schedule Task: AggregateRenderPackageAsyncTask
Process Task: AggregateRenderPackageAsyncTask
```

Changes
-----
One naive way of fixing this of course is to make this specific instance of ```DelegateBasedAsyncTask``` to have priority of ```TaskPriority.Critical``` so that it always come before ```UpdateGraphAsyncTask``` (which is of ```TaskPriority.AboveNormal```). However that does not always fix the problem, consider this case:

```
Schedule Task: DelegateBasedAsyncTask
Schedule Task: AggregateRenderPackageAsyncTask
Schedule Task: UpdateGraphAsyncTask ==> Scheduled with old EngineController
Process Task: DelegateBasedAsyncTask ==> Creates a new EngineController
Process Task: UpdateGraphAsyncTask ==> Update through the old EngineController
Schedule Task: UpdateRenderPackageAsyncTask ==> Scheduled with new EngineController
Process Task: UpdateRenderPackageAsyncTask ==> Gets nothing from the new EngineController
```

Also, the caller that schedules ```DelegateBasedAsyncTask``` cannot reliably determine if ```UpdateGraphAsyncTask``` is already in the task queue, because for what we know, it may have already been dispatched for execution (and therefore no longer exist in the queue). Even if ```DelegateBasedAsyncTask``` is on time to find out ```UpdateGraphAsyncTask``` is in the task queue, it is in no position to remove it from queue (otherwise it will have to make the owner of ```UpdateGraphAsyncTask``` to reschedule another one -- something that will overly complicates the design of ```DynamoScheduler``` unnecessarily).

The idea behind the fix is rather simple: ```RevitDynamoModel``` now handle the completion event of ```DelegateBasedAsyncTask```. When the task is completed, ```RevitDynamoModel``` calls its own ```RunExpression``` if ```DynamicRunEnabled == true```. ```RunExpression``` internally reschedules another ```UpdateGraphAsyncTask```, which is guaranteed to have the latest instance of ```EngineController```.

### Fixed scenario
With the fix, the following takes place. The completion of ```DelegateBasedAsyncTask``` (after the first ```UpdateGraphAsyncTask```) schedules the second ```UpdateGraphAsyncTask``` with the new ```EngineController```. Due to this fact, the second ```UpdateGraphAsyncTask``` is guaranteed to update the graph in the new ```EngineController```, resulting in right results being produced:

```
Schedule Task: DelegateBasedAsyncTask
Schedule Task: AggregateRenderPackageAsyncTask
Schedule Task: UpdateGraphAsyncTask ==> Scheduled with old EngineController
Process Task: UpdateGraphAsyncTask ==> Update through the old EngineController
Schedule Task: UpdateRenderPackageAsyncTask
Schedule Task: NotifyRenderPackagesReadyAsyncTask
Process Task: DelegateBasedAsyncTask ==> Creates a new EngineController
Schedule Task: UpdateGraphAsyncTask ==> Scheduled with the new EngineController
Process Task: UpdateGraphAsyncTask
Schedule Task: UpdateRenderPackageAsyncTask ==> Scheduled with the new EngineController
Schedule Task: NotifyRenderPackagesReadyAsyncTask
Process Task: AggregateRenderPackageAsyncTask
Process Task: UpdateRenderPackageAsyncTask ==> Gets the right value from new EngineController
Process Task: NotifyRenderPackagesReadyAsyncTask
Schedule Task: AggregateRenderPackageAsyncTask
Process Task: AggregateRenderPackageAsyncTask
```

**Interesting fact**: Note how ```UpdateRenderPackageAsyncTask``` and ```NotifyRenderPackagesReadyAsyncTask``` are both scheduled twice but processed only once. This is because of task queue compact functionality in ```DynamoScheduler``` that removes redundant tasks from its queue.

Notes to reviewers
-----
Hi guys, please take a look at this. With the above descriptions the fix should makes sense now. Thanks!

- [ ] @ke-yu (because you have debugged this yesterday)
- [ ] @lukechurch (because you know the design principles of scheduler)
- [ ] @ikeough (because you have debugged this before)

Defects fixed
-----

- [MAGN-5068](http://adsk-oss.myjetbrains.com/youtrack/issue/MAGN-5068) if Run Automatically On, third file onwards it executes to null
- [MAGN-5162](http://adsk-oss.myjetbrains.com/youtrack/issue/MAGN-5162) Some time graph doesn't execute after reopening file,if previous file was having unsaved changes.
- [MAGN-5219](http://adsk-oss.myjetbrains.com/youtrack/issue/MAGN-5219) Sometime graph executes correctly but there is no Background preview and all previews are null.