From df7763e53c24787fe90850a688df4568aa3e1e74 Mon Sep 17 00:00:00 2001
From: Peter Wehrfritz <peter.wehrfritz@mailbox.org>
Date: Wed, 17 Oct 2018 22:19:31 +0200
Subject: [PATCH] Fix order of observer and resource disposal of the Using and
 Finally operator, reported in #829

---
 .../Linq/Observable/Finally.cs                | 18 +++++------
 .../System.Reactive/Linq/Observable/Using.cs  | 14 +++++---
 .../Tests/Linq/Observable/UsingTest.cs        | 32 +++++++++++++++++++
 3 files changed, 49 insertions(+), 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
index f6b9edd8e8..48d04645c4 100644
--- 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
index 5177e04903..00a2c0f113 100644
--- 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
index 51a5a7cafe..e5d59692af 100644
--- 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);
+        }
     }
 }