Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thread safety? #32

Open
zsszatmari opened this issue Feb 2, 2016 · 7 comments
Open

Thread safety? #32

zsszatmari opened this issue Feb 2, 2016 · 7 comments

Comments

@zsszatmari
Copy link

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
Copy link
Owner

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
Copy link
Author

Wow, cool! Thanks again! Keep up the good work 👍

@zyzyxdev
Copy link

Hi, any news? Is this still in development?

@ossiv
Copy link

ossiv commented Feb 10, 2017

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
Copy link
Owner

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
Copy link

ossiv commented Mar 31, 2017

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
Copy link
Owner

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants