GuillaumeSalles / redux.NET

Redux.NET is a predictable state container for .NET apps. Inspired by https://github.com/reactjs/redux.
712 stars 86 forks source link

Thread safety? #32

Open zsszatmari opened 8 years ago

zsszatmari commented 8 years ago

Hi!

Awesome library! Is it thread safe? It seems to me, but I am not that familiar with System.Reactive, so there might be a catch, but better safe then sorry. Related question: Is there some kind of guarantee on which thread will the subscribed callback be called? For UI work, the main thread would be ideal, but one might dispatch actions in a background thread... It would be awesome if these things would be clearly documented. Thanks!

GuillaumeSalles commented 8 years ago

Hi,

Sorry for the delay, busy times for me. I thought the Rx Scan method was thread safe but apparently it's not. I made some modification and the store should now be thread safe (v0.5.0). I added a unit test to ensure that behavior.

Related question: Is there some kind of guarantee on which thread will the subscribed callback be called? For UI work, the main thread would be ideal, but one might dispatch actions in a background thread...

By default, the callbacks are executed synchronously just after the action is dispatched. So if we dispatch an action on the background thread and do some UI work inside a callback, a nasty cross thread access exception will be thrown. We can use the ObserveOn method to switch on the UI thread just before the callback is executed. Like this :

App.Store
    .ObserveOn(CoreDispatcherScheduler.Current)
    .Subscribe(counter => CounterRun.Text = counter.ToString());

You can find a full example here : https://github.com/GuillaumeSalles/redux.NET/tree/master/examples/multi-thread

To avoid the pain of adding "ObserveOn(CoreDispatcherScheduler.Current)" before each Subscribe, I will soon add the concept of Store Enhancer like in redux js. It will be easy to configure the store to dispatch on the background thread and subscribe on the UI thread by default. You can see a preview of it on the store-enhancers branch

I let the issue open to motivate me to add some documentation on the README...

Thanks for the kind words! I hope I answered your questions.

zsszatmari commented 8 years ago

Wow, cool! Thanks again! Keep up the good work :+1:

zyzyxdev commented 8 years ago

Hi, any news? Is this still in development?

ossiv commented 7 years ago

This is still a problem after 0.5.0. I have run into an issue where the reducers are run in the correct order, but the state subject's OnNext is called in reverse order for two actions dispatched close to one another from different threads. Fortunately I was able to fix this by just moving the dispatch so that it is run on the same thread as the other one.

GuillaumeSalles commented 7 years ago

Hi @ossiv,

Sorry for the delay. If you have a gist to reproduce the threads issue, I would me glad to take a look to fix it.

Thanks

ossiv commented 7 years ago

Hello, I have created this gist to try to reproduce the issue. The Debugger.Break() should never get hit since the value in the store is always increasing, but it does sometimes.

I think what may be happening is the following:

 private object InnerDispatch(object action)
        {
            lock (_syncRoot)
            {
                _lastState = _reducer(_lastState, action);
            }
            _stateSubject.OnNext(_lastState);
            return action;
        }

The lock statement, and thus the reducers, get run in the correct order, but after the lock, one thread stops executing and another one goes through both the lock block and calling _stateSubject.OnNext before the first one has a chance to call it.

GuillaumeSalles commented 7 years ago

Hey, you are totally right!

We are currently working on a full rewrite of redux.NET that I think should fix your issue. See #51 and #58.

The state will not be passed directly to the observer so GetState will always return the last state. It's not ready yet but I will add a unit-test based on your gist to make sure your issue is fixed.

Thanks a lot for your gist!