Use observable to signal update...

...in ExpressionObserver, instead of calling UpdateRoot. This should
avoid a leak.

src/Markup/Perspex.Markup.Xaml/Data/Binding.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See licence.md file in the project root for full license information.
 
 using System;
+using System.Reactive;
 using System.Reactive.Linq;
 using System.Reactive.Subjects;
 using Perspex.Controls;
@@ -216,15 +217,13 @@ namespace Perspex.Markup.Xaml.Data
 
             if (!targetIsDataContext)
             {
+                var update = target.GetObservable(Control.DataContextProperty)
+                    .Skip(1)
+                    .Select(_ => Unit.Default);
                 var result = new ExpressionObserver(
                     () => target.GetValue(Control.DataContextProperty),
-                    path);
+                    path,
-
+                    update);
-                /// TODO: Instead of doing this, make the ExpressionObserver accept an "update"
-                /// observable as doing it this way can will cause a leak in Binding as this 
-                /// observable is never unsubscribed.
-                target.GetObservable(Control.DataContextProperty).Subscribe(x =>
-                    result.UpdateRoot());
 
                 return result;
             }
@@ -245,19 +244,16 @@ namespace Perspex.Markup.Xaml.Data
         {
             Contract.Requires<ArgumentNullException>(target != null);
 
+            var update = target.GetObservable(Control.TemplatedParentProperty)
+                .Skip(1)
+                .Where(x => x != null)
+                .Take(1)
+                .Select(_ => Unit.Default);
+
             var result = new ExpressionObserver(
                 () => target.GetValue(Control.TemplatedParentProperty),
-                path);
+                path,
-
+                update);
-            if (target.GetValue(Control.TemplatedParentProperty) == null)
-            {
-                // TemplatedParent should only be set once, so only listen for the first non-null
-                // value.
-                target.GetObservable(Control.TemplatedParentProperty)
-                    .Where(x => x != null)
-                    .Take(1)
-                    .Subscribe(x => result.UpdateRoot());
-            }
 
             return result;
         }

src/Markup/Perspex.Markup/Data/ExpressionObserver.cs

@@ -5,6 +5,7 @@ using System;
 using System.Collections.Generic;
 using System.Reactive;
 using System.Reactive.Disposables;
+using System.Reactive.Linq;
 using System.Reactive.Subjects;
 using Perspex.Markup.Data.Plugins;
 
@@ -29,10 +30,11 @@ namespace Perspex.Markup.Data
         private readonly object _root;
         private readonly Func<object> _rootGetter;
         private readonly IObservable<object> _rootObservable;
+        private readonly IObservable<Unit> _update;
         private IDisposable _rootObserverSubscription;
+        private IDisposable _updateSubscription;
         private int _count;
         private readonly ExpressionNode _node;
-        private ISubject<object> _empty;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="ExpressionObserver"/> class.
@@ -78,12 +80,18 @@ namespace Perspex.Markup.Data
         /// </summary>
         /// <param name="rootGetter">A function which gets the root object.</param>
         /// <param name="expression">The expression.</param>
-        public ExpressionObserver(Func<object> rootGetter, string expression)
+        /// <param name="update">An observable which triggers a re-read of the getter.</param>
+        public ExpressionObserver(
+            Func<object> rootGetter, 
+            string expression,
+            IObservable<Unit> update)
         {
             Contract.Requires<ArgumentNullException>(rootGetter != null);
             Contract.Requires<ArgumentNullException>(expression != null);
+            Contract.Requires<ArgumentNullException>(update != null);
 
             _rootGetter = rootGetter;
+            _update = update;
 
             if (!string.IsNullOrWhiteSpace(expression))
             {
@@ -104,7 +112,11 @@ namespace Perspex.Markup.Data
         public bool SetValue(object value)
         {
             IncrementCount();
-            UpdateRoot();
+
+            if (_rootGetter != null && _node != null)
+            {
+                _node.Target = _rootGetter();
+            }
 
             try
             {
@@ -174,26 +186,6 @@ namespace Perspex.Markup.Data
             }
         }
 
-        /// <summary>
-        /// Causes the root object to be re-read from the root getter.
-        /// </summary>
-        /// TODO: Instead of doing this, make the object accept an "update" observable
-        /// as doing it this way can cause a leak in Binding.
-        public void UpdateRoot()
-        {
-            if (_count > 0 && _rootGetter != null)
-            {
-                if (_node != null)
-                {
-                    _node.Target = _rootGetter();
-                }
-                else
-                {
-                    _empty?.OnNext(_rootGetter());
-                }
-            }
-        }
-
         /// <inheritdoc/>
         protected override IDisposable SubscribeCore(IObserver<object> observer)
         {
@@ -211,12 +203,17 @@ namespace Perspex.Markup.Data
             }
             else
             {
-                if (_empty == null)
+                if (_update == null)
                 {
-                    _empty = new BehaviorSubject<object>(_rootGetter());
+                    return Observable.Never<object>().StartWith(_root).Subscribe(observer);
+                }
+                else
+                {
+                    return _update
+                        .Select(_ => _rootGetter())
+                        .StartWith(_rootGetter())
+                        .Subscribe(observer);
                 }
-
-                return _empty.Subscribe(observer);
             }
         }
 
@@ -227,6 +224,11 @@ namespace Perspex.Markup.Data
                 if (_rootGetter != null)
                 {
                     _node.Target = _rootGetter();
+
+                    if (_update != null)
+                    {
+                        _updateSubscription = _update.Subscribe(x => _node.Target = _rootGetter());
+                    }
                 }
                 else if (_rootObservable != null)
                 {
@@ -249,6 +251,12 @@ namespace Perspex.Markup.Data
                     _rootObserverSubscription = null;
                 }
 
+                if (_updateSubscription != null)
+                {
+                    _updateSubscription.Dispose();
+                    _updateSubscription = null;
+                }
+
                 _node.Target = null;
             }
         }

tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Lifetime.cs

@@ -46,6 +46,25 @@ namespace Perspex.Markup.UnitTests.Data
             Assert.NotEqual(Subscription.Infinite, source.Subscriptions[0].Unsubscribe);
         }
 
+        [Fact]
+        public void Should_Unsubscribe_From_Update_Observable()
+        {
+            var scheduler = new TestScheduler();
+            var update = scheduler.CreateColdObservable<Unit>();
+            var target = new ExpressionObserver(() => new { Foo = "foo" }, "Foo", update);
+            var result = new List<object>();
+
+            using (target.Subscribe(x => result.Add(x)))
+            using (target.Subscribe(_ => { }))
+            {
+                scheduler.Start();
+            }
+
+            Assert.Equal(new[] { "foo" }, result);
+            Assert.Equal(1, update.Subscriptions.Count);
+            Assert.NotEqual(Subscription.Infinite, update.Subscriptions[0].Unsubscribe);
+        }
+
         [Fact]
         public void Should_Set_Node_Target_To_Null_On_Unsubscribe()
         {

tests/Perspex.Markup.UnitTests/Data/ExpressionObserverTests_Property.cs

@@ -5,6 +5,7 @@ using System;
 using System.Collections.Generic;
 using System.Reactive;
 using System.Reactive.Linq;
+using System.Reactive.Subjects;
 using Microsoft.Reactive.Testing;
 using Perspex.Markup.Data;
 using Xunit;
@@ -193,13 +194,14 @@ namespace Perspex.Markup.UnitTests.Data
         public void Empty_Expression_Should_Track_Root()
         {
             var data = new Class1 { Foo = "foo" };
-            var target = new ExpressionObserver(() => data.Foo, "");
+            var update = new Subject<Unit>();
+            var target = new ExpressionObserver(() => data.Foo, "", update);
             var result = new List<object>();
 
             target.Subscribe(x => result.Add(x));
 
             data.Foo = "bar";
-            target.UpdateRoot();
+            update.OnNext(Unit.Default);
 
             Assert.Equal(new[] { "foo", "bar" }, result);
         }
@@ -286,14 +288,15 @@ namespace Perspex.Markup.UnitTests.Data
             var first = new Class1 { Foo = "foo" };
             var second = new Class1 { Foo = "bar" };
             var root = first;
-            var target = new ExpressionObserver(() => root, "Foo");
+            var update = new Subject<Unit>();
+            var target = new ExpressionObserver(() => root, "Foo", update);
             var result = new List<object>();
             var sub = target.Subscribe(x => result.Add(x));
 
             root = second;
-            target.UpdateRoot();
+            update.OnNext(Unit.Default);
             root = null;
-            target.UpdateRoot();
+            update.OnNext(Unit.Default);
 
             Assert.Equal(new[] { "foo", "bar", PerspexProperty.UnsetValue }, result);
 

tests/Perspex.Markup.UnitTests/Perspex.Markup.UnitTests.csproj

@@ -84,7 +84,7 @@
     <Compile Include="ControlLocatorTests.cs" />
     <Compile Include="Data\ExpressionNodeBuilderTests.cs" />
     <Compile Include="Data\ExpressionNodeBuilderTests_Errors.cs" />
-    <Compile Include="Data\ExpressionObserverTests.cs" />
+    <Compile Include="Data\ExpressionObserverTests_Lifetime.cs" />
     <Compile Include="Data\ExpressionObserverTests_Indexer.cs" />
     <Compile Include="Data\ExpressionObserverTests_Negation.cs" />
     <Compile Include="Data\ExpressionObserverTests_Observable.cs" />