Align safe observers (#603)

danielcweber · web-flow · commit 0c5a53f4d474 · 2018-06-18T17:08:56.000+02:00
diff --git a/Rx.NET/Source/src/System.Reactive/AnonymousSafeObserver.cs b/Rx.NET/Source/src/System.Reactive/AnonymousSafeObserver.cs
@@ -19,12 +19,11 @@ namespace System.Reactive
     /// that accept delegates for the On* handlers. By doing the fusion, we make the call stack depth shorter which
     /// helps debugging and some performance.
     /// </summary>
-    internal sealed class AnonymousSafeObserver<T> : ISafeObserver<T>
+    internal sealed class AnonymousSafeObserver<T> : SafeObserver<T>
     {
         private readonly Action<T> _onNext;
         private readonly Action<Exception> _onError;
         private readonly Action _onCompleted;
-        private IDisposable _disposable;
 
         private int isStopped;
 
@@ -35,7 +34,7 @@ public AnonymousSafeObserver(Action<T> onNext, Action<Exception> onError, Action
             _onCompleted = onCompleted;
         }
 
-        public void OnNext(T value)
+        public override void OnNext(T value)
         {
             if (isStopped == 0)
             {
@@ -53,7 +52,7 @@ public void OnNext(T value)
             }
         }
 
-        public void OnError(Exception error)
+        public override void OnError(Exception error)
         {
             if (Interlocked.Exchange(ref isStopped, 1) == 0)
             {
@@ -64,7 +63,7 @@ public void OnError(Exception error)
             }
         }
 
-        public void OnCompleted()
+        public override void OnCompleted()
         {
             if (Interlocked.Exchange(ref isStopped, 1) == 0)
             {
@@ -74,15 +73,5 @@ public void OnCompleted()
                 }
             }
         }
-
-        public void SetResource(IDisposable resource)
-        {
-            Disposable.SetSingle(ref _disposable, resource);
-        }
-
-        public void Dispose()
-        {
-            Disposable.TryDispose(ref _disposable);
-        }
     }
 }
diff --git a/Rx.NET/Source/src/System.Reactive/Internal/AutoDetachObserver.cs b/Rx.NET/Source/src/System.Reactive/Internal/AutoDetachObserver.cs
@@ -3,11 +3,10 @@
 // See the LICENSE file in the project root for more information. 
 
 using System.Reactive.Disposables;
-using System.Threading;
 
 namespace System.Reactive
 {
-    internal sealed class AutoDetachObserver<T> : ObserverBase<T>
+    internal sealed class AutoDetachObserver<T> : ObserverBase<T>, ISafeObserver<T>
     {
         private readonly IObserver<T> _observer;
 
@@ -18,9 +17,9 @@ public AutoDetachObserver(IObserver<T> observer)
             _observer = observer;
         }
 
-        public IDisposable Disposable
+        public void SetResource(IDisposable resource)
         {
-            set => Disposables.Disposable.SetSingle(ref _disposable, value);
+            Disposable.SetSingle(ref _disposable, resource);
         }
 
         protected override void OnNextCore(T value)
diff --git a/Rx.NET/Source/src/System.Reactive/Internal/ObserverWithToken.cs b/Rx.NET/Source/src/System.Reactive/Internal/ObserverWithToken.cs
diff --git a/Rx.NET/Source/src/System.Reactive/Internal/Producer.cs b/Rx.NET/Source/src/System.Reactive/Internal/Producer.cs
@@ -46,7 +46,7 @@ public IDisposable SubscribeRaw(IObserver<TSource> observer, bool enableSafeguar
             //
             if (enableSafeguard)
             {
-                var safeObserver = SafeObserver<TSource>.Create(observer);
+                var safeObserver = SafeObserver<TSource>.Wrap(observer);
                 safeObserver.SetResource(subscription);
                 observer = safeObserver;
             }
@@ -100,7 +100,7 @@ public IDisposable SubscribeRaw(IObserver<TTarget> observer, bool enableSafeguar
             //
             if (enableSafeguard)
             {
-                observer = safeObserver = SafeObserver<TTarget>.Create(observer);
+                observer = safeObserver = SafeObserver<TTarget>.Wrap(observer);
             }
 
             var sink = CreateSink(observer);
diff --git a/Rx.NET/Source/src/System.Reactive/Internal/SafeObserver.cs b/Rx.NET/Source/src/System.Reactive/Internal/SafeObserver.cs
@@ -11,70 +11,85 @@ namespace System.Reactive
     // its implementation aspects.
     //
 
-    internal sealed class SafeObserver<TSource> : ISafeObserver<TSource>
+    internal abstract class SafeObserver<TSource> : ISafeObserver<TSource>
     {
-        private readonly IObserver<TSource> _observer;
-
-        private IDisposable _disposable;
-
-        public static ISafeObserver<TSource> Create(IObserver<TSource> observer)
+        private sealed class WrappingSafeObserver : SafeObserver<TSource>
         {
-            if (observer is AnonymousObserver<TSource> a)
+            private readonly IObserver<TSource> _observer;
+
+            public WrappingSafeObserver(IObserver<TSource> observer)
             {
-                return a.MakeSafe();
+                _observer = observer;
             }
-            else
+
+            public override void OnNext(TSource value)
             {
-                return new SafeObserver<TSource>(observer);
+                var __noError = false;
+                try
+                {
+                    _observer.OnNext(value);
+                    __noError = true;
+                }
+                finally
+                {
+                    if (!__noError)
+                    {
+                        Dispose();
+                    }
+                }
             }
-        }
 
-        private SafeObserver(IObserver<TSource> observer)
-        {
-            _observer = observer;
-        }
-
-        public void OnNext(TSource value)
-        {
-            var __noError = false;
-            try
+            public override void OnError(Exception error)
             {
-                _observer.OnNext(value);
-                __noError = true;
+                using (this)
+                {
+                    _observer.OnError(error);
+                }
             }
-            finally
+
+            public override void OnCompleted()
             {
-                if (!__noError)
+                using (this)
                 {
-                    Dispose();
+                    _observer.OnCompleted();
                 }
             }
         }
 
-        public void OnError(Exception error)
+        public static ISafeObserver<TSource> Wrap(IObserver<TSource> observer)
         {
-            using (this)
+            if (observer is AnonymousObserver<TSource> a)
             {
-                _observer.OnError(error);
+                return a.MakeSafe();
             }
-        }
-
-        public void OnCompleted()
-        {
-            using (this)
+            else
             {
-                _observer.OnCompleted();
+                return new WrappingSafeObserver(observer);
             }
         }
 
+        private IDisposable _disposable;
+        
+        public abstract void OnNext(TSource value);
+
+        public abstract void OnError(Exception error);
+
+        public abstract void OnCompleted();
+
         public void SetResource(IDisposable resource)
         {
             Disposable.SetSingle(ref _disposable, resource);
         }
 
         public void Dispose()
         {
-            Disposable.TryDispose(ref _disposable);
+            Dispose(true);
+        }
+
+        protected virtual void Dispose(bool disposing)
+        {
+            if (disposing)
+                Disposable.TryDispose(ref _disposable);
         }
     }
 }
diff --git a/Rx.NET/Source/src/System.Reactive/Observable.Extensions.cs b/Rx.NET/Source/src/System.Reactive/Observable.Extensions.cs
@@ -259,7 +259,7 @@ private static void Subscribe_<T>(this IObservable<T> source, IObserver<T> obser
             {
                 if (!token.IsCancellationRequested)
                 {
-                    var consumer = new ObserverWithToken<T>(observer);
+                    var consumer = SafeObserver<T>.Wrap(observer);
 
                     //
                     // [OK] Use of unsafe Subscribe: exception during Subscribe doesn't orphan CancellationTokenRegistration.
diff --git a/Rx.NET/Source/src/System.Reactive/ObservableBase.cs b/Rx.NET/Source/src/System.Reactive/ObservableBase.cs
@@ -54,7 +54,7 @@ public IDisposable Subscribe(IObserver<T> observer)
             {
                 try
                 {
-                    autoDetachObserver.Disposable = SubscribeCore(autoDetachObserver);
+                    autoDetachObserver.SetResource(SubscribeCore(autoDetachObserver));
                 }
                 catch (Exception exception)
                 {
@@ -79,7 +79,7 @@ private IDisposable ScheduledSubscribe(IScheduler _, AutoDetachObserver<T> autoD
         {
             try
             {
-                autoDetachObserver.Disposable = SubscribeCore(autoDetachObserver);
+                autoDetachObserver.SetResource(SubscribeCore(autoDetachObserver));
             }
             catch (Exception exception)
             {
diff --git a/Rx.NET/Source/src/System.Reactive/Platforms/Desktop/Linq/QueryLanguage.Remoting.cs b/Rx.NET/Source/src/System.Reactive/Platforms/Desktop/Linq/QueryLanguage.Remoting.cs
@@ -85,7 +85,7 @@ public SerializableObservable(RemotableObservable<T> remotableObservable)
 
             public IDisposable Subscribe(IObserver<T> observer)
             {
-                var consumer = new ObserverWithToken<T>(observer);
+                var consumer = SafeObserver<T>.Wrap(observer);
 
                 //
                 // [OK] Use of unsafe Subscribe: non-pretentious transparent wrapping through remoting; exception coming from the remote object is not re-routed.

Original file line number	Diff line number	Diff line change
`@@ -19,12 +19,11 @@ namespace System.Reactive`
`19`	`19`	`/// that accept delegates for the On* handlers. By doing the fusion, we make the call stack depth shorter which`
`20`	`20`	`/// helps debugging and some performance.`
`21`	`21`	`/// </summary>`
`22`		`- internal sealed class AnonymousSafeObserver<T> : ISafeObserver<T>`
	`22`	`+ internal sealed class AnonymousSafeObserver<T> : SafeObserver<T>`
`23`	`23`	`{`
`24`	`24`	`private readonly Action<T> _onNext;`
`25`	`25`	`private readonly Action<Exception> _onError;`
`26`	`26`	`private readonly Action _onCompleted;`
`27`		`- private IDisposable _disposable;`
`28`	`27`
`29`	`28`	`private int isStopped;`
`30`	`29`
`@@ -35,7 +34,7 @@ public AnonymousSafeObserver(Action<T> onNext, Action<Exception> onError, Action`
`35`	`34`	`_onCompleted = onCompleted;`
`36`	`35`	`}`
`37`	`36`
`38`		`- public void OnNext(T value)`
	`37`	`+ public override void OnNext(T value)`
`39`	`38`	`{`
`40`	`39`	`if (isStopped == 0)`
`41`	`40`	`{`
`@@ -53,7 +52,7 @@ public void OnNext(T value)`
`53`	`52`	`}`
`54`	`53`	`}`
`55`	`54`
`56`		`- public void OnError(Exception error)`
	`55`	`+ public override void OnError(Exception error)`
`57`	`56`	`{`
`58`	`57`	`if (Interlocked.Exchange(ref isStopped, 1) == 0)`
`59`	`58`	`{`
`@@ -64,7 +63,7 @@ public void OnError(Exception error)`
`64`	`63`	`}`
`65`	`64`	`}`
`66`	`65`
`67`		`- public void OnCompleted()`
	`66`	`+ public override void OnCompleted()`
`68`	`67`	`{`
`69`	`68`	`if (Interlocked.Exchange(ref isStopped, 1) == 0)`
`70`	`69`	`{`
`@@ -74,15 +73,5 @@ public void OnCompleted()`
`74`	`73`	`}`
`75`	`74`	`}`
`76`	`75`	`}`
`77`		`-`
`78`		`- public void SetResource(IDisposable resource)`
`79`		`- {`
`80`		`- Disposable.SetSingle(ref _disposable, resource);`
`81`		`- }`
`82`		`-`
`83`		`- public void Dispose()`
`84`		`- {`
`85`		`- Disposable.TryDispose(ref _disposable);`
`86`		`- }`
`87`	`76`	`}`
`88`	`77`	`}`
Original file line number	Diff line number	Diff line change
`@@ -3,11 +3,10 @@`
`3`	`3`	`// See the LICENSE file in the project root for more information.`
`4`	`4`
`5`	`5`	`using System.Reactive.Disposables;`
`6`		`-using System.Threading;`
`7`	`6`
`8`	`7`	`namespace System.Reactive`
`9`	`8`	`{`
`10`		`- internal sealed class AutoDetachObserver<T> : ObserverBase<T>`
	`9`	`+ internal sealed class AutoDetachObserver<T> : ObserverBase<T>, ISafeObserver<T>`
`11`	`10`	`{`
`12`	`11`	`private readonly IObserver<T> _observer;`
`13`	`12`
`@@ -18,9 +17,9 @@ public AutoDetachObserver(IObserver<T> observer)`
`18`	`17`	`_observer = observer;`
`19`	`18`	`}`
`20`	`19`
`21`		`- public IDisposable Disposable`
	`20`	`+ public void SetResource(IDisposable resource)`
`22`	`21`	`{`
`23`		`- set => Disposables.Disposable.SetSingle(ref _disposable, value);`
	`22`	`+ Disposable.SetSingle(ref _disposable, resource);`
`24`	`23`	`}`
`25`	`24`
`26`	`25`	`protected override void OnNextCore(T value)`
Original file line number	Diff line number	Diff line change
`@@ -46,7 +46,7 @@ public IDisposable SubscribeRaw(IObserver<TSource> observer, bool enableSafeguar`
`46`	`46`	`//`
`47`	`47`	`if (enableSafeguard)`
`48`	`48`	`{`
`49`		`- var safeObserver = SafeObserver<TSource>.Create(observer);`
	`49`	`+ var safeObserver = SafeObserver<TSource>.Wrap(observer);`
`50`	`50`	`safeObserver.SetResource(subscription);`
`51`	`51`	`observer = safeObserver;`
`52`	`52`	`}`
`@@ -100,7 +100,7 @@ public IDisposable SubscribeRaw(IObserver<TTarget> observer, bool enableSafeguar`
`100`	`100`	`//`
`101`	`101`	`if (enableSafeguard)`
`102`	`102`	`{`
`103`		`- observer = safeObserver = SafeObserver<TTarget>.Create(observer);`
	`103`	`+ observer = safeObserver = SafeObserver<TTarget>.Wrap(observer);`
`104`	`104`	`}`
`105`	`105`
`106`	`106`	`var sink = CreateSink(observer);`
Original file line number	Diff line number	Diff line change
`@@ -11,70 +11,85 @@ namespace System.Reactive`
`11`	`11`	`// its implementation aspects.`
`12`	`12`	`//`
`13`	`13`
`14`		`- internal sealed class SafeObserver<TSource> : ISafeObserver<TSource>`
	`14`	`+ internal abstract class SafeObserver<TSource> : ISafeObserver<TSource>`
`15`	`15`	`{`
`16`		`- private readonly IObserver<TSource> _observer;`
`17`		`-`
`18`		`- private IDisposable _disposable;`
`19`		`-`
`20`		`- public static ISafeObserver<TSource> Create(IObserver<TSource> observer)`
	`16`	`+ private sealed class WrappingSafeObserver : SafeObserver<TSource>`
`21`	`17`	`{`
`22`		`- if (observer is AnonymousObserver<TSource> a)`
	`18`	`+ private readonly IObserver<TSource> _observer;`
	`19`	`+`
	`20`	`+ public WrappingSafeObserver(IObserver<TSource> observer)`
`23`	`21`	`{`
`24`		`- return a.MakeSafe();`
	`22`	`+ _observer = observer;`
`25`	`23`	`}`
`26`		`- else`
	`24`	`+`
	`25`	`+ public override void OnNext(TSource value)`
`27`	`26`	`{`
`28`		`- return new SafeObserver<TSource>(observer);`
	`27`	`+ var __noError = false;`
	`28`	`+ try`
	`29`	`+ {`
	`30`	`+ _observer.OnNext(value);`
	`31`	`+ __noError = true;`
	`32`	`+ }`
	`33`	`+ finally`
	`34`	`+ {`
	`35`	`+ if (!__noError)`
	`36`	`+ {`
	`37`	`+ Dispose();`
	`38`	`+ }`
	`39`	`+ }`
`29`	`40`	`}`
`30`		`- }`
`31`	`41`
`32`		`- private SafeObserver(IObserver<TSource> observer)`
`33`		`- {`
`34`		`- _observer = observer;`
`35`		`- }`
`36`		`-`
`37`		`- public void OnNext(TSource value)`
`38`		`- {`
`39`		`- var __noError = false;`
`40`		`- try`
	`42`	`+ public override void OnError(Exception error)`
`41`	`43`	`{`
`42`		`- _observer.OnNext(value);`
`43`		`- __noError = true;`
	`44`	`+ using (this)`
	`45`	`+ {`
	`46`	`+ _observer.OnError(error);`
	`47`	`+ }`
`44`	`48`	`}`
`45`		`- finally`
	`49`	`+`
	`50`	`+ public override void OnCompleted()`
`46`	`51`	`{`
`47`		`- if (!__noError)`
	`52`	`+ using (this)`
`48`	`53`	`{`
`49`		`- Dispose();`
	`54`	`+ _observer.OnCompleted();`
`50`	`55`	`}`
`51`	`56`	`}`
`52`	`57`	`}`
`53`	`58`
`54`		`- public void OnError(Exception error)`
	`59`	`+ public static ISafeObserver<TSource> Wrap(IObserver<TSource> observer)`
`55`	`60`	`{`
`56`		`- using (this)`
	`61`	`+ if (observer is AnonymousObserver<TSource> a)`
`57`	`62`	`{`
`58`		`- _observer.OnError(error);`
	`63`	`+ return a.MakeSafe();`
`59`	`64`	`}`
`60`		`- }`
`61`		`-`
`62`		`- public void OnCompleted()`
`63`		`- {`
`64`		`- using (this)`
	`65`	`+ else`
`65`	`66`	`{`
`66`		`- _observer.OnCompleted();`
	`67`	`+ return new WrappingSafeObserver(observer);`
`67`	`68`	`}`
`68`	`69`	`}`
`69`	`70`
	`71`	`+ private IDisposable _disposable;`
	`72`	`+`
	`73`	`+ public abstract void OnNext(TSource value);`
	`74`	`+`
	`75`	`+ public abstract void OnError(Exception error);`
	`76`	`+`
	`77`	`+ public abstract void OnCompleted();`
	`78`	`+`
`70`	`79`	`public void SetResource(IDisposable resource)`
`71`	`80`	`{`
`72`	`81`	`Disposable.SetSingle(ref _disposable, resource);`
`73`	`82`	`}`
`74`	`83`
`75`	`84`	`public void Dispose()`
`76`	`85`	`{`
`77`		`- Disposable.TryDispose(ref _disposable);`
	`86`	`+ Dispose(true);`
	`87`	`+ }`
	`88`	`+`
	`89`	`+ protected virtual void Dispose(bool disposing)`
	`90`	`+ {`
	`91`	`+ if (disposing)`
	`92`	`+ Disposable.TryDispose(ref _disposable);`
`78`	`93`	`}`
`79`	`94`	`}`
`80`	`95`	`}`
Original file line number	Diff line number	Diff line change
`@@ -259,7 +259,7 @@ private static void Subscribe_<T>(this IObservable<T> source, IObserver<T> obser`
`259`	`259`	`{`
`260`	`260`	`if (!token.IsCancellationRequested)`
`261`	`261`	`{`
`262`		`- var consumer = new ObserverWithToken<T>(observer);`
	`262`	`+ var consumer = SafeObserver<T>.Wrap(observer);`
`263`	`263`
`264`	`264`	`//`
`265`	`265`	`// [OK] Use of unsafe Subscribe: exception during Subscribe doesn't orphan CancellationTokenRegistration.`
Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ public IDisposable Subscribe(IObserver<T> observer)`
`54`	`54`	`{`
`55`	`55`	`try`
`56`	`56`	`{`
`57`		`- autoDetachObserver.Disposable = SubscribeCore(autoDetachObserver);`
	`57`	`+ autoDetachObserver.SetResource(SubscribeCore(autoDetachObserver));`
`58`	`58`	`}`
`59`	`59`	`catch (Exception exception)`
`60`	`60`	`{`
`@@ -79,7 +79,7 @@ private IDisposable ScheduledSubscribe(IScheduler _, AutoDetachObserver<T> autoD`
`79`	`79`	`{`
`80`	`80`	`try`
`81`	`81`	`{`
`82`		`- autoDetachObserver.Disposable = SubscribeCore(autoDetachObserver);`
	`82`	`+ autoDetachObserver.SetResource(SubscribeCore(autoDetachObserver));`
`83`	`83`	`}`
`84`	`84`	`catch (Exception exception)`
`85`	`85`	`{`
Original file line number	Diff line number	Diff line change
`@@ -85,7 +85,7 @@ public SerializableObservable(RemotableObservable<T> remotableObservable)`
`85`	`85`
`86`	`86`	`public IDisposable Subscribe(IObserver<T> observer)`
`87`	`87`	`{`
`88`		`- var consumer = new ObserverWithToken<T>(observer);`
	`88`	`+ var consumer = SafeObserver<T>.Wrap(observer);`
`89`	`89`
`90`	`90`	`//`
`91`	`91`	`// [OK] Use of unsafe Subscribe: non-pretentious transparent wrapping through remoting; exception coming from the remote object is not re-routed.`