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

ReactiveCommand is sometimes unintuitive #585

Closed
onovotny opened this Issue May 9, 2014 · 10 comments

Comments

Projects
None yet
2 participants
@onovotny
Copy link
Member

onovotny commented May 9, 2014

Trying to use ReactiveCommand on RxUI 6 (5.99.5), I get cross thread exceptions even if RxUI.MainThreadScheduler is specified.

@onovotny

This comment has been minimized.

Copy link
Member

onovotny commented May 9, 2014

Something is really weird. In my App.OnWindowCreated method, I added the following:

var disp =  CoreDispatcherScheduler.Current;
RxApp.MainThreadScheduler = disp;

In the debugger, I verified that disp's internall CoreDispatcher was not null and HasThreadAccess was true.

I still get cross-thread exceptions though. Looking at the call stack, I see this:

Dtu.Client.ViewModels.DLL!Dtu.Client.ViewModels.ArticleListViewModel.ArticleTab.set(Dtu.Client.ViewModels.ArticleListTab value) Line 81 C#
Dtu.Client.ViewModels.DLL!Dtu.Client.ViewModels.ArticleListViewModel..ctor.AnonymousMethod__5(object o) Line 64 C#
ReactiveUI.DLL!ReactiveUI.ReactiveCommand.Create.AnonymousMethod__12() Unknown
System.Reactive.Linq.DLL!System.Reactive.Linq.QueryLanguage.ToAsync.AnonymousMethod__13a() Unknown
System.Reactive.Core.DLL!System.Reactive.Concurrency.Scheduler.Invoke(System.Reactive.Concurrency.IScheduler scheduler, System.Action action) Unknown
System.Reactive.PlatformServices.DLL!System.Reactive.Concurrency.TaskPoolScheduler.Schedule<System.Action>.AnonymousMethod__0() Unknown
mscorlib.dll!System.Threading.Tasks.Task.InnerInvoke() Unknown
mscorlib.dll!System.Threading.Tasks.Task.Execute() Unknown
mscorlib.dll!System.Threading.Tasks.Task.ExecutionContextCallback(object obj) Unknown
mscorlib.dll!System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx) Unknown
mscorlib.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx) Unknown
mscorlib.dll!System.Threading.Tasks.Task.ExecuteWithThreadLocal(ref System.Threading.Tasks.Task currentTaskSlot) Unknown
mscorlib.dll!System.Threading.Tasks.Task.ExecuteEntry(bool bPreventDoubleExecution) Unknown
mscorlib.dll!System.Threading.Tasks.Task.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem() Unknown
mscorlib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch() Unknown
mscorlib.dll!System.Threading._ThreadPoolWaitCallback.PerformWaitCallback() Unknown

Notice the Rx Scheduler.Invoke doesn't actually change to another thread as the call-stack goes down to a thread pool.

@onovotny

This comment has been minimized.

Copy link
Member

onovotny commented May 9, 2014

Ok, more fishy stuff -- looks like the TaskPoolScheduler is being used instead of the CoreDispatcherScheduler.

@paulcbetts

This comment has been minimized.

Copy link
Member

paulcbetts commented May 9, 2014

@onovotny Can you paste in a snippet of the code you're using ?

@onovotny

This comment has been minimized.

Copy link
Member

onovotny commented May 9, 2014

I'm doing this:

 var cmd = ReactiveCommand.Create(o =>
{
     var en = (ArticleListTab)Enum.Parse(typeof(ArticleListTab), (string)o);
    ArticleTab = en;
}, RxApp.MainThreadScheduler);
TabSelectedCommand = cmd;

Then in the XAML, I'm using a data trigger bound to an IsSelected property to execute the command. This is silly, but I'm binding it to a Synfusion TabControl so I get a command when a tab is selected.

@onovotny

This comment has been minimized.

Copy link
Member

onovotny commented May 9, 2014

I think the scheduler is being ignored for the execution. This is totally Not Good, because I need to be able to specify my own scheduler for that. Even if not the UI thread, maybe I have a throttled/limiting scheduler? Using taskpool isn't always right and shouldn't be hard-coded (as it appears to be in the ReactiveCommand.Create* factories.

@paulcbetts

This comment has been minimized.

Copy link
Member

paulcbetts commented May 9, 2014

The scheduler used to run async operations is always TaskpoolScheduler, the scheduler parameter is the scheduler to deliver results to. I think here, you want to write it this way instead:

var cmd = ReactiveCommand.Create();
cmd.Subscribe(o => {
    var en = (ArticleListTab)Enum.Parse(typeof(ArticleListTab), (string)o);
    ArticleTab = en;
});
@onovotny

This comment has been minimized.

Copy link
Member

onovotny commented May 9, 2014

Doesn't work. The parameter gets lost--- where's 'o'?

This is super unintuitive. While the default might be the TaskPoolScheduler, it's certainly possible I need a throttled resource? What if I want to supply my own scheduler that does that throttling? It seems like there needs to be two scheduler parameters -- one to deliver results and one to execute?

Not only that, but I used a Create() method, not a CreateAsync(). I expect the execute to be sync on the same thread.

@paulcbetts

This comment has been minimized.

Copy link
Member

paulcbetts commented May 9, 2014

Oops, added 'o' back in.

Not only that, but I used a Create() method, not a CreateAsync()

That's legit - I do think the New ReactiveCommand is way less intuitive than the old one, at least for people used to the old one. I might end up making the New Hotness into its own command and bringing back the old one too.

@onovotny

This comment has been minimized.

Copy link
Member

onovotny commented May 9, 2014

Thx, that worked. I think part of the confusion stems from the fact there is a scheduler parameter available but it's not really clear where it's used.

Also, I do like the pattern of the new ReactiveCommand in general, but I also don't think it's safe to assume that the execute should always be on the task pool. Maybe there by default, but it should be overrideable.

@paulcbetts paulcbetts added this to the ReactiveUI 6.0 milestone May 10, 2014

@paulcbetts paulcbetts changed the title Cross-thread exceptions with ReactiveCommand in RxUI 6 on WinRT 8.1 ReactiveCommand is sometimes unintuitive May 27, 2014

@paulcbetts

This comment has been minimized.

Copy link
Member

paulcbetts commented Jun 13, 2014

New Create overloads should cover this. Case dismissed!

@paulcbetts paulcbetts closed this Jun 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment