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

fix: assign VM from correct thread in RoutedViewHost #1281

Merged
merged 6 commits into from Feb 21, 2017

Conversation

Projects
None yet
4 participants
@PureWeen
Copy link
Contributor

PureWeen commented Feb 19, 2017

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

On iOS specifically a "UIKitThreadAccessException" is thrown from
https://github.com/reactiveui/ReactiveUI/blob/develop/src/ReactiveUI/PropertyBinding.cs#L966

When this VM is assigned
https://github.com/reactiveui/ReactiveUI/blob/master/src/ReactiveUI.XamForms/XamForms/RoutedViewHost.cs#L80

I'm pretty sure the async await from the previous Select causes the Do to execute on a thread that's not the UI Thread which then causes a thread access exception.

What is the current behavior? (You can also link to an open issue here)
#1271

Project to recreate issue here
https://github.com/PureWeen/ReactiveUI.XamlForms.Sample/tree/Viewmodel_not_firing_propertyChanged

It's a bit of a rare issue because usually all the Views on the Navigation stack have the VMs set but in a case where the stack is deserialized that won't be the case.. Also this only seems to cause issues on iOS.

What is the new behavior (if this is a feature change)?
Ensure that the Viewmodel will be assigned to the View from the UI Thread

Does this PR introduce a breaking change?
It shouldn't

Please check if the PR fulfills these requirements

Other information:

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 19, 2017

Coverage Status

Coverage remained the same at 65.938% when pulling e9a235f on PureWeen:1271_ensure_vm_assigned_from_MainThreadScheduler_when_popping_vms_xam_forms into bf6e1a4 on reactiveui:develop.

@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 19, 2017

Thanks @PureWeen. Can you check to make sure you're not modifying the navigation stack from a non-UI thread. That's the scenario that I think would explain your issue. I suspect there's a better way to solve this problem if that's the case.

Basically, the expectation is that things with UI thread affinity should be done on th UI thread. But we should be validating that with some kind of assertion rather than failing with an unhelpful exception. An ObserveOn incurs a cost for code that is already doing the right thing (i.e. already running on the UI thread).

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 19, 2017

I'm pretty sure everything I'm doing up until that point isn't too crazy. The code I'm calling for the POP is just this

NavigateBack = ReactiveCommand.CreateFromObservable(
				() => HostScreen.Router.NavigateBack.Execute(Unit.Default));

I was also curious at what point it "jumped ship"

If I break point the async SelectMany block above the DO then it's on the UI THREAD

image

But then it hops over to the Thread Pool

image

In my very non-scientific experience code that follows "async" blocks in Observables don't seem to hop back on the UI Thread.... For example in most of my internal Navigation code I always ObserveOn the MainScheduler afterwards because of this

That being said instead of doing an ObserveOn just moving the setter up into the async block also achieves the same result and probably without the added performance cost

image

@ChrisPulman

This comment has been minimized.

Copy link

ChrisPulman commented Feb 20, 2017

have you tried:
await this.PopAsync(i == x.Delta - 1).ConfigureAwait(true);

This should return it back to the same thread.

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 20, 2017

Yea I mean it's still on the UI Thread after the awaits run within that block of work. In my last screen shot you'll see if I just assign the VM after the awaits instead of inside the Do then it all works fine. It's when it leaves the async block inside the SelectMany and then the work inside the Do gets scheduled on a Thread Pool thread.

Here's a screen shot though of adding .ConfigureAWait(true)'s

You'll notice the DO still runs on a ThreadPool

image

If I just use that commented out code in the finally block then it all works great so I'm thinking that's the way to go

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 20, 2017

If I were to venture a guess as to why it's like this.

My guess would be that it's due to the fact that inside Rx the continuation of the task is managed via ContinueWith opposed to an await so it's not going to save state and pick up where it left off. The work after the task is just going to get scheduled on I'm guessing the DefaultScheduler.

https://github.com/Reactive-Extensions/Rx.NET/blob/master/Rx.NET/Source/System.Reactive.Linq/Reactive/Linq/Observable/SelectMany.cs#L1565

@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 20, 2017

What I'm finding particularly confusing about all this is that I'm using RoutedViewHost in an iOS app and not experiencing this issue. On top of that, it's utterly bizarre to me that the finally block executes on the correct thread, but the Do action does not. After all, the tasks being awaited have completed before the finally block, so they're obviously continuing on the correct thread.

Another theory: could you try setting a breakpoint on the lambda inside .Where(x => x.Delta > 0) and check which thread you're on?

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 20, 2017

I think it's the difference with how "await" works vs "ContinueWith" . Inside the SelectMany it's using "await" which does a bunch of stuff to save the state and make sure that it comes back on the same thread. Whereas the surrounding implementation from Rx.NET uses ContinueWith on the Task to signal to the observer that it has completed. It's going to execute that ContinueWith on the Thread Pool because no scheduler was supplied to the ContinueWith meaning that Do is going to execute from the Thread Pool. At least that's my working theory at this point.

http://blog.stephencleary.com/2013/10/continuewith-is-dangerous-too.html

Here's a quick example of what I mean

The awaited code stays on the UI Thread
image

But the ContinueWith runs on the Thread Pool
image

As far as why you're not seeing the issue. If I were to guess I would say that maybe it's because you're never quite triggering the effect? This will only occur if setting the ViewModel from a Pop causes the property to change. Which will only really happen if the ViewModel is null. In my case I only came across this when I started the application on the second page and the VM hadn't been assigned yet to the first page. If that first page already has the VM set then setting the ViewModel to the same value does nothing so no exception or anything of interest occurs.

if you check out this sample here
https://github.com/PureWeen/ReactiveUI.XamlForms.Sample/tree/Viewmodel_not_firing_propertyChanged

I insert two pages on the stack and if you click the back button you'll notice on iOS the ViewModel fails to get set on the main page.

Break point you requested
image

The Do afterwards

image

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 20, 2017

Also it might be even more rare that it comes up because I've noticed that the exception only happens when there's a binding that will trigger the exception

For example in my recreation if I comment this line out
this.OneWayBind(ViewModel, vm => vm.MainText, v => v.lbl.Text);

And leave this line in
this.OneWayBind(ViewModel, vm => vm.NavigateToSecondPage, v => v.btn.Command);
The ViewModel assigns and works perfectly.

But if I bring that Label binding back then it no longer works. Which if what I'm saying is true about it setting on the ThreadPool makes sense but I'm just bringing it up because that might be another reason why you haven't observed it. Because there seems to only be a few types of cases that will throw an exception. Whereas other cases are resilient to the VM being set from the ThreadPool

@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 20, 2017

Makes me wonder why we're using SelectMany with async and tasks at all. If we just switched it over to Rx so we're not mixing TPL and Rx (like I always advise people against doing) then it should all just work?

Sorry to have to back and forth like this, but it's difficult when I don't have a repro. To be honest, I'm at work so don't have the time to try right now.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 20, 2017

Coverage Status

Coverage remained the same at 65.938% when pulling 72e0edd on PureWeen:1271_ensure_vm_assigned_from_MainThreadScheduler_when_popping_vms_xam_forms into 01c6af2 on reactiveui:develop.

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 20, 2017

Yea it's interesting how oddly that all plays together :-/ I pushed a new commit that uses ToObservable and Concat in place of the TPLs and found it does the job :-) That Do now runs on the UI Thread and iOS is happy.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 20, 2017

Coverage Status

Coverage increased (+0.7%) to 66.688% when pulling e823493 on PureWeen:1271_ensure_vm_assigned_from_MainThreadScheduler_when_popping_vms_xam_forms into 01c6af2 on reactiveui:develop.

@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 20, 2017

Ah, excellent! So the explanation here must be that SelectMany, when given a Task, calls ContinueWith(false) on that Task. That explains why the finally block still executes on the correct thread, but the Do block does not. It does not explain why I never ran into this problem, but I'm happy to leave that mystery unsolved :)

One more thing and I'll be happy to merge this: please change from Select + Concat to just a SelectMany (but returning observables, not tasks). Outwardly, they should behave the same (in this particular case), but the former is a bit confusing to read.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 20, 2017

Coverage Status

Coverage increased (+0.7%) to 66.688% when pulling 66e01c5 on PureWeen:1271_ensure_vm_assigned_from_MainThreadScheduler_when_popping_vms_xam_forms into 01c6af2 on reactiveui:develop.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 21, 2017

Coverage Status

Coverage remained the same at 66.688% when pulling 6a3575b on PureWeen:1271_ensure_vm_assigned_from_MainThreadScheduler_when_popping_vms_xam_forms into d9d36e5 on reactiveui:develop.

@kentcb kentcb changed the title bug: ensure vm on xam forms RoutedViewHost gets assigned from MainThr… fix: assign VM from correct thread in RoutedViewHost Feb 21, 2017

@kentcb kentcb added this to the vNext milestone Feb 21, 2017

@kentcb kentcb merged commit 823298d into reactiveui:develop Feb 21, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage remained the same at 66.688%
Details
@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 21, 2017

Great! Thanks very much for pushing this through, @PureWeen - much appreciated.

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 22, 2017

Sigh :-( Actually I'm pretty sure I did this wrong....... In my haste I forgot to actually return the Observable's from that block so they don't actually become part of that chain. This is why now it's staying on the UIThread because the DO is just running on the Immediate Scheduler... Looking at the Rx source code any non completed task will have it's result scheduled inside a ContinueWith which will be on the Thread Pool

https://github.com/Reactive-Extensions/Rx.NET/blob/master/Rx.NET/Source/System.Reactive.Linq/Reactive/Threading/Tasks/TaskObservableExtensions.cs#L149

I've been running a few permutations of Obs => Task interactions and can't really find a great combination that will cause the continuing Observable to stay on the UI Thread unless the Task for some reason has already completed.

I was messing with ToObservable in my own project and wasn't getting the same result of having the later code running on the UI Thread which is why I came across this

Something like this works though it's not that pretty

 Subject<string> returnValue = new Subject<string>();
            Observable.StartAsync(async () =>
            {
                var result = await Xamarin.Forms.Application.Current.MainPage.DisplayActionSheet(title, cancel, destruction, buttons);
                returnValue.OnNext(result ?? cancel);
                returnValue.OnCompleted();

            });

            return returnValue;

At this point though I feel like the best option would be to just roll back to the awaits and assign the VM in the finally block.

@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 22, 2017

Sorry, I should have spotted the missing return statements 🙃

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