reactiveui / ReactiveUI

An advanced, composable, functional reactive model-view-viewmodel framework for all .NET platforms that is inspired by functional reactive programming. ReactiveUI allows you to abstract mutable state away from your user interfaces, express the idea around a feature in one readable place and improve the testability of your application.
https://www.reactiveui.net
MIT License
8.1k stars 1.12k forks source link

NullReferenceException potential #254

Closed bradphelan closed 10 years ago

bradphelan commented 11 years ago

I've modified some code in RUI only to get a null reference exception because a Schedular could not be found. Tracing back to where the null came from

static RxApp()
{
    TaskpoolScheduler = Scheduler.TaskPool;

    DeferredScheduler = new WaitForDispatcherScheduler(() => {
        Type dispatcherType = 
            Type.GetType("System.Reactive.Windows.Threading.DispatcherScheduler, System.Reactive.Windows.Threading", false) ?? 
            Type.GetType("System.Reactive.Windows.Threading.CoreDispatcherScheduler, System.Reactive.Windows.Threading", false);

        if (dispatcherType != null) {
            return (IScheduler)dispatcherType.GetProperty("Current").GetMethod.Invoke(null, null);
        }

        return null;
    });

has a fallthrough at the end to return null if not Scheduler could be found. I suspect that the return null at the end was just to make the compiler happy that all code paths were covered but is really an error.

Question: Is it a valid code path for this to return null? If not can we throw an exception here.

The code where the NullReferenceException pops up is here

    public class WaitForDispatcherScheduler : IScheduler
    {
        ...
        public IDisposable Schedule<TState>(TState state, Func<IScheduler, TState, IDisposable> action)
        {
            return attemptToCreateScheduler().Schedule(state, action);
        }

I would argue that

return attemptToCreateScheduler().Schedule(state, action);

is evil. attempt suggests that there may be failure but the return value is not checked and thus a NullReferenceException will occur.

I won't fix it straight away. Will wait for feedback as I'm not really sure what the behaviour should be for this code.

BTW. Life would be easier if we used the F# option class to indicate an empty return rather than null. I have my own implementation of it in C# called Maybe. Makes the code cleaner and you can clearly specify via types that you might return something or nothing

Maybe<IScheduler> attemptCreateScheduler(){}

and you can consume it a number of ways

return attemptCreateScheduler().Else<SomeException>(); 
return attemptCreateScheduler().Else(Scheduler.Default);
return attemptCreateScheduler().Else(()=>Scheduler.Default);

The point is you have to go through hoops to get a null reference exception when using Maybe.

bradphelan commented 11 years ago

Something evil is going on in the tests. The above code generates me a NRE when I run the test BindToShouldntInitiallySetToNull by itself but the code passes when I run all the tests together. Note I stashed all my changes and this failure is on rxui5-master.

Test Name:  BindToShouldntInitiallySetToNull
Test FullName:  ReactiveUI.Tests.PropertyBindingTest.BindToShouldntInitiallySetToNull
Test Source:    c:\Users\phelan\workspace\ReactiveUI\ReactiveUI.Tests\PropertyBindingTest.cs : line 342
Test Outcome:   Failed
Test Duration:  0:00:38,01

Result Message: System.NullReferenceException : Object reference not set to an instance of an object.
Result StackTrace:  
at ReactiveUI.WaitForDispatcherScheduler.Schedule[TState](TState state, Func`3 action) in c:\Users\phelan\workspace\ReactiveUI\ReactiveUI\WaitForDispatcherScheduler.cs:line 27
   at System.Reactive.Concurrency.Scheduler.Schedule(IScheduler scheduler, Action action)
   at System.Reactive.Linq.Observαble.Return`1._.Run()
   at System.Reactive.Linq.Observαble.Return`1.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.TailRecursiveSink`1.MoveNext()
   at System.Reactive.Concurrency.AsyncLock.Wait(Action action)
   at System.Reactive.TailRecursiveSink`1.<Run>b__0(Action self)
   at System.Reactive.Concurrency.Scheduler.<Schedule>b__45(Action`1 _action, Action`1 self)
   at System.Reactive.Concurrency.Scheduler.<>c__DisplayClass50`1.<InvokeRec1>b__4d(TState state1)
   at System.Reactive.Concurrency.Scheduler.InvokeRec1[TState](IScheduler scheduler, Pair`2 pair)
   at System.Reactive.Concurrency.ImmediateScheduler.Schedule[TState](TState state, Func`3 action)
   at System.Reactive.Concurrency.Scheduler.Schedule[TState](IScheduler scheduler, TState state, Action`2 action)
   at System.Reactive.Concurrency.Scheduler.Schedule(IScheduler scheduler, Action`1 action)
   at System.Reactive.TailRecursiveSink`1.Run(IEnumerable`1 sources)
   at System.Reactive.Linq.Observαble.Concat`1.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.Linq.Observαble.Select`2.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.TailRecursiveSink`1.MoveNext()
   at System.Reactive.Concurrency.AsyncLock.Wait(Action action)
   at System.Reactive.TailRecursiveSink`1.<Run>b__0(Action self)
   at System.Reactive.Concurrency.Scheduler.<Schedule>b__45(Action`1 _action, Action`1 self)
   at System.Reactive.Concurrency.Scheduler.<>c__DisplayClass50`1.<InvokeRec1>b__4d(TState state1)
   at System.Reactive.Concurrency.Scheduler.InvokeRec1[TState](IScheduler scheduler, Pair`2 pair)
   at System.Reactive.Concurrency.ImmediateScheduler.Schedule[TState](TState state, Func`3 action)
   at System.Reactive.Concurrency.Scheduler.Schedule[TState](IScheduler scheduler, TState state, Action`2 action)
   at System.Reactive.Concurrency.Scheduler.Schedule(IScheduler scheduler, Action`1 action)
   at System.Reactive.TailRecursiveSink`1.Run(IEnumerable`1 sources)
   at System.Reactive.Linq.Observαble.Concat`1.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.Linq.Observαble.Switch`1._.OnNext(IObservable`1 value)
   at System.Reactive.Linq.Observαble.Select`2._.OnNext(TSource value)
   at System.Reactive.Linq.Observαble.Return`1._.Invoke()
   at System.Reactive.Concurrency.Scheduler.Invoke(IScheduler scheduler, Action action)
   at System.Reactive.Concurrency.ImmediateScheduler.Schedule[TState](TState state, Func`3 action)
   at System.Reactive.Concurrency.Scheduler.Schedule(IScheduler scheduler, Action action)
   at System.Reactive.Linq.Observαble.Return`1._.Run()
   at System.Reactive.Linq.Observαble.Return`1.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.Linq.Observαble.Select`2.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.Linq.Observαble.Switch`1._.Run()
   at System.Reactive.Linq.Observαble.Switch`1.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.Linq.Observαble.Where`1.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.Linq.Observαble.Select`2.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.Linq.Observαble.DistinctUntilChanged`2.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.Linq.Observαble.Select`2.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.Linq.Observαble.CombineLatest`3._.Run()
   at System.Reactive.Linq.Observαble.CombineLatest`3.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.ObservableExtensions.SubscribeSafe[T](IObservable`1 source, IObserver`1 observer)
   at System.Reactive.Linq.Observαble.Where`1.Run(IObserver`1 observer, IDisposable cancel, Action`1 setSink)
   at System.Reactive.Producer`1.Run(IScheduler _, State x)
   at System.Reactive.Concurrency.ScheduledItem`2.InvokeCore()
   at System.Reactive.Concurrency.ScheduledItem`1.Invoke()
   at System.Reactive.Concurrency.CurrentThreadScheduler.Trampoline.Run(SchedulerQueue`1 queue)
   at System.Reactive.Concurrency.CurrentThreadScheduler.Schedule[TState](TState state, TimeSpan dueTime, Func`3 action)
   at System.Reactive.Concurrency.LocalScheduler.Schedule[TState](TState state, Func`3 action)
   at System.Reactive.Producer`1.SubscribeRaw(IObserver`1 observer, Boolean enableSafeguard)
   at System.Reactive.Producer`1.Subscribe(IObserver`1 observer)
   at System.ObservableExtensions.Subscribe[T](IObservable`1 source, Action`1 onNext, Action`1 onError)
   at ReactiveUI.PropertyBinderImplementation.bindToDirect[TTarget,TValue](IObservable`1 This, TTarget target, Expression`1 property, Func`1 fallbackValue) in c:\Users\phelan\workspace\ReactiveUI\ReactiveUI\PropertyBinding.cs:line 1260
   at ReactiveUI.PropertyBinderImplementation.OneWayBind[TViewModel,TView,TVMProp,TVProp](TViewModel viewModel, TView view, Expression`1 vmProperty, Expression`1 viewProperty, Func`1 fallbackValue, Object conversionHint) in c:\Users\phelan\workspace\ReactiveUI\ReactiveUI\PropertyBinding.cs:line 1057
   at ReactiveUI.BindingMixins.OneWayBind[TViewModel,TView,TVMProp,TVProp](TView view, TViewModel viewModel, Expression`1 vmProperty, Expression`1 viewProperty, Func`1 fallbackValue, Object conversionHint) in c:\Users\phelan\workspace\ReactiveUI\ReactiveUI\PropertyBinding.cs:line 238
   at ReactiveUI.Tests.PropertyBindingTest.BindToShouldntInitiallySetToNull() in c:\Users\phelan\workspace\ReactiveUI\ReactiveUI.Tests\PropertyBindingTest.cs:line 346
bradphelan commented 11 years ago

I've tried this a number of times. run all the test passes. run selected test the test fails. The unit test runner should not have any state between tests.

anaisbetts commented 11 years ago

I suspect that it's because the ReactiveUI Service Locator setup gets to run, which reconfigures DeferredScheduler.

bradphelan commented 11 years ago

I'm not really sure what that means. How to fix it?

carlowahlstedt commented 11 years ago

I seem to be running across this issue in my code for Xamarin.iOS. Was there ever a determined resolution?

anaisbetts commented 11 years ago

@carlowahlstedt This is a separate issue with some versions of Xamarin.iOS where Type.GetType doesn't work like it does on other platforms. Paste this into your AppDelegate:

public override bool FinishedLaunching(UIApplication app, NSDictionary options)
{
    // NB: GrossHackAlertTiem™:
    //
    // Monotouch appears to not load assemblies when you request them
    // via Type.GetType, unlike every other platform (even
    // Xamarin.Android). So, we've got to manually do what RxUI and
    // Akavache would normally do for us
    var r = new ModernDependencyResolver();
    (new ReactiveUI.Registrations()).Register((f,t) => r.Register(f, t));
    (new ReactiveUI.Cocoa.Registrations()).Register((f,t) => r.Register(f, t));
    (new ReactiveUI.Mobile.Registrations()).Register((f,t) => r.Register(f, t));

    RxApp.DependencyResolver = r;
    (new Akavache.Registrations()).Register(r.Register);
    (new Akavache.Mobile.Registrations()).Register(r.Register);
    (new Akavache.Sqlite3.Registrations()).Register(r.Register);
}
jlaanstra commented 10 years ago

This should be fixed by using the above code. Closing this.