From cfab9f9be37092ab355aa546ff75267e54b14e6d Mon Sep 17 00:00:00 2001 From: Peter Wehrfritz Date: Wed, 17 Oct 2018 22:19:31 +0200 Subject: [PATCH 1/4] 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 { private readonly Action _finallyAction; - private IDisposable _sourceDisposable; - public _(Action finallyAction, IObserver observer) : base(observer) { @@ -35,19 +33,19 @@ public _(Action finallyAction, IObserver observer) public override void Run(IObservable 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 observer) public void Run(Using parent) { var source = default(IObservable); + 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(exception).SubscribeSafe(this)); - - return; + source = Observable.Throw(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()))) + .Finally(() => order += "4") + .Subscribe() + .Dispose(); + + Assert.Equal("1234", order); + } } } From 31e6aa86bb7ca9828d1c89597f68ab066483cf14 Mon Sep 17 00:00:00 2001 From: Peter Wehrfritz Date: Fri, 19 Oct 2018 00:45:42 +0200 Subject: [PATCH 2/4] Save two allocations in the finally operator --- .../Linq/Observable/Finally.cs | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 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 48d04645c4..c3a23333e1 100644 --- a/Rx.NET/Source/src/System.Reactive/Linq/Observable/Finally.cs +++ b/Rx.NET/Source/src/System.Reactive/Linq/Observable/Finally.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System.Reactive.Disposables; +using System.Threading; namespace System.Reactive.Linq.ObservableImpl { @@ -24,6 +25,7 @@ public Finally(IObservable source, Action finallyAction) internal sealed class _ : IdentitySink { private readonly Action _finallyAction; + private IDisposable _sourceDisposable; public _(Action finallyAction, IObserver observer) : base(observer) @@ -33,19 +35,43 @@ public _(Action finallyAction, IObserver observer) public override void Run(IObservable source) { - var subscription = source.SubscribeSafe(this); + var d = source.SubscribeSafe(this); - SetUpstream(Disposable.Create(() => + if (Interlocked.CompareExchange(ref _sourceDisposable, d, null) == BooleanDisposable.True) { + // The Dispose(bool) methode was already called before the + // subscription could be assign, hence the subscription + // needs to be diposed here and the action needs to be invoked. try { - subscription.Dispose(); + d.Dispose(); } finally { _finallyAction(); } - })); + } + } + + protected override void Dispose(bool disposing) + { + base.Dispose(disposing); + + if (disposing) + { + var d = Interlocked.Exchange(ref _sourceDisposable, BooleanDisposable.True); + if (d != BooleanDisposable.True && d != null) + { + try + { + d.Dispose(); + } + finally + { + _finallyAction(); + } + } + } } } } From 0ee203521eefd1d57dd94032accdff5dc26a102c Mon Sep 17 00:00:00 2001 From: Peter Wehrfritz Date: Fri, 19 Oct 2018 21:32:49 +0200 Subject: [PATCH 3/4] Test disposal order of Finally --- .../Tests/Linq/Observable/FinallyTest.cs | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/FinallyTest.cs b/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/FinallyTest.cs index 9c662938a2..a9beb3ff3e 100644 --- a/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/FinallyTest.cs +++ b/Rx.NET/Source/tests/Tests.System.Reactive/Tests/Linq/Observable/FinallyTest.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using System.Reactive; using System.Reactive.Linq; using Microsoft.Reactive.Testing; using Xunit; @@ -142,5 +143,48 @@ public void Finally_Throw() ); } + [Fact] + public void Finally_DisposeOrder_Empty() + { + var order = ""; + Observable + .Empty() + .Finally(() => order += "1") + .Finally(() => order += "2") + .Finally(() => order += "3") + .Subscribe(); + + Assert.Equal("123", order); + } + + [Fact] + public void Finally_DisposeOrder_Return() + { + var order = ""; + Observable + .Return(Unit.Default) + .Finally(() => order += "1") + .Finally(() => order += "2") + .Finally(() => order += "3") + .Subscribe(); + + Assert.Equal("123", order); + } + + [Fact] + public void Finally_DisposeOrder_Never() + { + var order = ""; + var d = Observable + .Never() + .Finally(() => order += "1") + .Finally(() => order += "2") + .Finally(() => order += "3") + .Subscribe(); + + d.Dispose(); + + Assert.Equal("123", order); + } } } From c19d489c86b6c159202270bce4c84394404e41a7 Mon Sep 17 00:00:00 2001 From: Oren Novotny Date: Thu, 1 Aug 2019 16:58:04 -0400 Subject: [PATCH 4/4] bump to 4.1.6 --- Rx.NET/Source/version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rx.NET/Source/version.json b/Rx.NET/Source/version.json index 5d2679c466..c322faa40a 100644 --- a/Rx.NET/Source/version.json +++ b/Rx.NET/Source/version.json @@ -1,5 +1,5 @@ { - "version": "4.1.5", + "version": "4.1.6", "publicReleaseRefSpec": [ "^refs/heads/master$", // we release out of master "^refs/heads/rel/v\\d+\\.\\d+" // we also release branches starting with rel/vN.N