Fix order of observer and resource disposal of the Using and Finally …

…operator, reported in #829
dotnet · Oct 17, 2018 · df7763e · akarnokd · Oct 18, 2018 · df7763e
1 parent 338741d
commit df7763e
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 15 deletions.
diff --git a/Rx.NET/Source/src/System.Reactive/Linq/Observable/Finally.cs b/Rx.NET/Source/src/System.Reactive/Linq/Observable/Finally.cs
@@ -25,8 +25,6 @@ internal sealed class _ : IdentitySink<TSource>
         {
             private readonly Action _finallyAction;
 
-            private IDisposable _sourceDisposable;
-
             public _(Action finallyAction, IObserver<TSource> observer)
                 : base(observer)
             {
@@ -35,19 +33,19 @@ public _(Action finallyAction, IObserver<TSource> observer)
 
             public override void Run(IObservable<TSource> source)
             {
-                Disposable.SetSingle(ref _sourceDisposable, source.SubscribeSafe(this));
-            }
+                var subscription = source.SubscribeSafe(this);
 
-            protected override void Dispose(bool disposing)
-            {
-                if (disposing)
+                SetUpstream(Disposable.Create(() =>
                 {
-                    if (Disposable.TryDispose(ref _sourceDisposable))
+                    try
+                    {
+                        subscription.Dispose();
+                    }
+                    finally
                     {
                         _finallyAction();
                     }
-                }
-                base.Dispose(disposing);
+                }));
             }
         }
     }

diff --git a/Rx.NET/Source/src/System.Reactive/Linq/Observable/Using.cs b/Rx.NET/Source/src/System.Reactive/Linq/Observable/Using.cs
@@ -34,33 +34,37 @@ public _(IObserver<TSource> observer)
             public void Run(Using<TSource, TResource> parent)
             {
                 var source = default(IObservable<TSource>);
+                var disposable = Disposable.Empty;
                 try
                 {
                     var resource = parent._resourceFactory();
                     if (resource != null)
                     {
-                        Disposable.SetSingle(ref _disposable, resource);
+                        disposable = resource;
                     }
 
                     source = parent._observableFactory(resource);
                 }
                 catch (Exception exception)
                 {
-                    SetUpstream(Observable.Throw<TSource>(exception).SubscribeSafe(this));
-
-                    return;
+                    source = Observable.Throw<TSource>(exception);
                 }
 
+                // It is important to set the disposable resource after
+                // Run(). In the synchronous case this would else dispose
+                // the the resource before the source subscription.
                 Run(source);
+                Disposable.SetSingle(ref _disposable, disposable);
             }
 
             protected override void Dispose(bool disposing)
             {
+                base.Dispose(disposing);
+
                 if (disposing)
                 {
                     Disposable.TryDispose(ref _disposable);
                 }
-                base.Dispose(disposing);
             }
         }
     }

diff --git a/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/UsingTest.cs b/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/UsingTest.cs
@@ -3,6 +3,8 @@
 // See the LICENSE file in the project root for more information. 
 
 using System;
+using System.Reactive;
+using System.Reactive.Disposables;
 using System.Reactive.Linq;
 using Microsoft.Reactive.Testing;
 using ReactiveTests.Dummies;
@@ -295,5 +297,35 @@ public void Using_ThrowResourceUsage()
             );
         }
 
+        [Fact]
+        public void Using_NestedCompleted()
+        {
+            var order = "";
+
+            Observable.Using(() => Disposable.Create(() => order += "3"),
+                _ => Observable.Using(() => Disposable.Create(() => order += "2"),
+                    __ => Observable.Using(() => Disposable.Create(() => order += "1"),
+                        ___ => Observable.Return(Unit.Default))))
+                .Finally(() => order += "4")
+                .Subscribe();
+
+            Assert.Equal("1234", order);
+        }
+
+        [Fact]
+        public void Using_NestedDisposed()
+        {
+            var order = "";
+
+            Observable.Using(() => Disposable.Create(() => order += "3"),
+                _ => Observable.Using(() => Disposable.Create(() => order += "2"),
+                    __ => Observable.Using(() => Disposable.Create(() => order += "1"),
+                        ___ => Observable.Never<Unit>())))
+                .Finally(() => order += "4")
+                .Subscribe()
+                .Dispose();
+
+            Assert.Equal("1234", order);
+        }
     }
 }