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

[BUG] UWP - SingleWindowDispatcherScheduler causes issues in XamlIsland #2154

Closed
jasonwurzel opened this issue Sep 3, 2019 · 12 comments · Fixed by #2251
Closed

[BUG] UWP - SingleWindowDispatcherScheduler causes issues in XamlIsland #2154

jasonwurzel opened this issue Sep 3, 2019 · 12 comments · Fixed by #2251
Labels
bug

Comments

@jasonwurzel
Copy link
Contributor

@jasonwurzel jasonwurzel commented Sep 3, 2019

Describe the bug
My UWP project lives inside a XamlIsland, that is, I have a WPF application having a WindowsXamlHost that in turn hosts a UWP control. Since #2100 any access to RxApp.MainThreadScheduler results in an Exception.
The root cause is that in the newly introduced SingleWindowDispatcherScheduler the dispatcher is fetched via CoreApplication.Views[0].Dispatcher, but this is not present inside the XamlIsland Context. CoreApplication.Views.Count is > 0, but subsequently accessing .Dispatcher results in an Exception.

Steps To Reproduce

see https://github.com/jasonwurzel/XamlIslandDispatcherProblem for a repo. Start either ControlPlaygroundPackage (WPF - XamlIsland - Uwp) or UwpApp (UWP App) (in Debug - x64). Starting UwpApp results in no exception, starting the WPF App results in an exception when CoreApplication.Views[0].Dispatcher is accessed.

  • Min Windows version is 1903
  • dotnet core sdk 3.0.100-preview5-011568 is expected to be present

Expected behavior
Accessing RxApp.MainThreadScheduler should not result in an exception. If it is correct that the Views in the Views enumeration have no dispatcher in the XamlIsland case, we have to find a workaround in that special case.

Additional context
I'm not sure whether the views in the Views enumeration should have a Dispatcher present or not. I'll check with the Windows.Toolkit guys as well.
For simplicity's sake I ommitted referencing the actual ReactiveUI package in my repro (XamlIslands with 3rd party nuget references is still uncharted territory, unfortunately and requires some - more - fiddling with the csproj files.)

@jasonwurzel jasonwurzel added the bug label Sep 3, 2019
@open-collective-bot

This comment has been minimized.

Copy link

@open-collective-bot open-collective-bot bot commented Sep 3, 2019

Hey @jasonwurzel 👋,

Thank you for opening an issue. We will get back to you as soon as we can. Also, check out our Open Collective and consider contributing financially.

https://opencollective.com/reactiveui

PS.: We offer priority support for all financial contributors. Don't forget to add priority label once you start contributing 😄

An advanced, composable, functional reactive model-view-viewmodel framework for all .NET platforms!
@glennawatson

This comment has been minimized.

Copy link
Contributor

@glennawatson glennawatson commented Sep 3, 2019

I'll have a look on the weekend anyway. I would like to get this solved for rxui 10 release

@jasonwurzel

This comment has been minimized.

Copy link
Contributor Author

@jasonwurzel jasonwurzel commented Sep 3, 2019

That's great! Maybe I'll have an answer until then from windows-toolkit ( windows-toolkit/Microsoft.Toolkit.Win32#174 )

@jasonwurzel

This comment has been minimized.

Copy link
Contributor Author

@jasonwurzel jasonwurzel commented Sep 4, 2019

Alexandre was swift to answer my issue on windows-toolkit, see here for his answer.
So basically this behaviour is by design and we have to find another way.
Linked to that I have a question, could one supply a custom Scheduler? Can I override the PlatformRegistrations mechanism?

@glennawatson

This comment has been minimized.

Copy link
Contributor

@glennawatson glennawatson commented Sep 4, 2019

Well. All the platform registrations do is add registrations to splat so potentially you can add your own registration and override the rxui with your own worse case. I will see about doing that fix for the weekend regardless.

@jasonwurzel

This comment has been minimized.

Copy link
Contributor Author

@jasonwurzel jasonwurzel commented Sep 18, 2019

Hey Glenn,
It seems I can't prevent the bug by means of setting my own MainThreadScheduler because it directly gets set to the static property RxApp.MainThreadScheduler = new SingleWindowDispatcherScheduler(); in PlatformRegistrations. Thus it doesn't use DI here, the Scheduler gets instantiated immediately, and as the exception gets thrown in the ctor there's nothing I can do about it. So my solution will be to stay on a RxUI version that still works (before this PR introducing the bug).

On another note: Shouldn't it be possible to register the MainThreadScheduler in DI so anyone can override it (using the given registerFunction in PlatformRegistrations)? If you think so, I'll try to do a PR. (same for TaskPoolScheduler, of course)

@glennawatson

This comment has been minimized.

Copy link
Contributor

@glennawatson glennawatson commented Sep 18, 2019

@RLittlesII is replacing the init to work much closer to the way a lot of Microsoft init works for Rx 11. The issue with the current approach is it's expensive to use reflection for all loaded assemblies which that change would require.

@glennawatson

This comment has been minimized.

Copy link
Contributor

@glennawatson glennawatson commented Oct 15, 2019

I think this is fixed by #2215

@jasonwurzel

This comment has been minimized.

Copy link
Contributor Author

@jasonwurzel jasonwurzel commented Nov 19, 2019

After being on RxUI 9.13.1 for a while (the last version without SingleWindowDispatcherScheduler), I have some time to finally look into this again. Unfortunately it isn't fixed by #2215 . The reason is that in the XamlIsland case, any access to the CoreApplication.Views collection throws an exception. (be it Views[0] or Views.FirstOrDefault()).

#2215 did not replace the logic in the constructor, so the exception is still raised (Views.Count > 0 yields true, yet accessing Views[0] throws). Ah, the wonders of XamlIslands...

I'm a bit hesitant to fix it. Basically, my fix would be to fall back to using the old logic for uap, directly setting CoreDispatcherScheduler.Current, relying on some mechanism to detect we are inside a XamlIsland. In the worst case this would be a boolean flag somewhere (RxApp).
But as @glennawatson said RLittlesII will modify the init anyway, so maybe it's best to wait until RxUI 11 and do the work then (registering some custom init logic or whatever will be possible then). XamlIsland will remain more of an edge case I guess.

@glennawatson glennawatson reopened this Nov 19, 2019
@glennawatson

This comment has been minimized.

Copy link
Contributor

@glennawatson glennawatson commented Nov 19, 2019

@RLittlesII is going through some real life stuff at the moment so might be best if we can fix it now.

@jasonwurzel jasonwurzel mentioned this issue Nov 20, 2019
0 of 2 tasks complete
glennawatson added a commit that referenced this issue Nov 20, 2019
type: fix 
accessing CoreApplication.Views throws in XamlIslands, thus in this case get the CoreDispatcher by alternate means

issue: #2154
@glennawatson

This comment has been minimized.

Copy link
Contributor

@glennawatson glennawatson commented Nov 20, 2019

I pushed out a release with your change in it this morning. Thanks :)

@jasonwurzel

This comment has been minimized.

Copy link
Contributor Author

@jasonwurzel jasonwurzel commented Nov 20, 2019

yes, thank you very much for the fast release, already testing 👍

jasonwurzel pushed a commit to jasonwurzel/ReactiveUI that referenced this issue Nov 21, 2019
this fix is related to reactiveui#2154 , some more testing showed that the former bugfix also has to be in place in ScheduleOnDispatcherNow
glennawatson added a commit that referenced this issue Nov 21, 2019
…2255)

this fix is related to #2154 , some more testing showed that the former bugfix also has to be in place in ScheduleOnDispatcherNow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.