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

fix: Replace default `MainThreadScheduler` on UAP #2100

Merged
merged 3 commits into from Jul 1, 2019

Conversation

Projects
None yet
4 participants
@hinterlandsupplyco
Copy link
Contributor

commented Jun 29, 2019

What kind of change does this PR introduce?
This replaces the usage of WaitForDispatcherScheduler and CoreDispatcherScheduler with a scheduler SingleWindowDispatcherScheduler that explicitly dispatches to the main app window regardless of how many windows are created by the application.

What is the current behavior?
See #2092, #2034 for more info. Should also resolve or at least improve the situation in #1001. Every window created by the CoreApplication gets it's own shiny new UI thread and CoreDispatcher. Example of this would be applications which use casting, ShareTarget, ProjectionManager or are just snazzy enough to have an app with multiple windows.

Because WaitForDispatcherScheduler uses a factory to get an instance of CoreDispatcherScheduler and CoreDispatcherScheduler uses Window.Current to obtain a CoreDispatcher there are multiple ways for MainThreadScheduler to be hijacked by a second window. For instance, if the thread being used by that window is returned to the thread pool (e.g. - by that window being closed or launching your main window from a ShareTargetActivated) then forever that [ThreadStatic] instance of MainThreadScheduler will attempt to dispatch to an non-existent window. This will surface in thread marshaling exceptions which will very likely crash the user's application (or cause nasty UI issues like controls which no longer update).

Of course, in these situations most devs will simply inject a scheduler into their view models for anything that used on a separate window. But this still won't block the hijacking because if code from the main window is ever executed on the thread separate window and then attempts to get an instance of MainThreadScheduler you could still have this problem. An example of this is code executing from the UI thread of window 2 updates an observable which is subscribed to on a view model or service being used in window 1. If in that code now in window 1 attempts to ObserveOn(RxApp.MainThreadScheduler) and MainThreadScheduler does not have an instance for that thread (which it likely wouldn't since we aren't using it on our other window in this example) it will grab the wrong dispatcher because for that thread Window.Current.Dispatcher is not in fact null, but is the dispatcher for window 2.

Since that is a very likely scenario in Rx, the problem should be solved in ReactiveUI.

What is the new behavior?

SingleWindowDispatcherScheduler does not use CoreDispatcherScheduler as its underlying scheduler, but does use code and patterns from that implementation.

It also does not use Window.Current to find the correct dispatcher. Instead it uses CoreApplication.Views to find the dispatcher. This has several advantages: 1) It is always available and you can get the correct dispatcher from any thread, 2) It is not possible to get the wrong dispatcher because your app's main window will always exist at CoreApplication.Views[0]

What might this PR break?
If for some reason a user was expressly relying on RxApp.MainThreadScheduler being of a specific type they will be discouraged from doing that ever again.

Please check if the PR fulfills these requirements

  • [] Tests for the changes have been added (for bug fixes / features)
  • [] Docs have been added / updated (for bug fixes / features)

Other information:
Unfortunately this code is not testable in a unit test, but then again neither are other platform schedulers (see NSRunloopScheduler). Since most of the actual dispatching code is taken from CoreDispatcherScheduler I think we are ok.

A separate PR will be added to the website documentation to make it VERY clear that ReactiveUI supports one window UWP environments only out of the box and suggest appropriate workarounds for multiple windows.

Also tested that this will not cause a regression on fixes from c5b9462 (original issue: #1221)

Trevor Kennedy
fix: Replace default `MainThreadScheduler` on UAP
See #2092, #2032 for more info. This replaces the usage of
`WaitForDispatcherScheduler` and `CoreDispatcherScheduler` with a
scheduler that explicitly dispatches to the main app window regardless
of how many windows are created by the application.

@hinterlandsupplyco hinterlandsupplyco requested a review from reactiveui/uwp-team as a code owner Jun 29, 2019

@dnfclas

This comment has been minimized.

Copy link

commented Jun 29, 2019

CLA assistant check
All CLA requirements met.

@hinterlandsupplyco

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

@glennawatson I did give a multiple window scheduler the good ole' college try but after about a week of testing I do not think it's possible. See an attempt here: https://gist.github.com/hinterlandsupplyco/c8e0396f6d837fdc9cd2a5b00688b451

Gist
This is an attempt to build a scheduler for `ReactiveUI` which can handle multiple windows. It doesn't work. - MultipleWindowDispatcherScheduler.cs
@glennawatson

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2019

I'm going to get multiple reviewers on this one who have some uwp experience. So hang in there.

@worldbeater
Copy link
Member

left a comment

Just tried the new SingleWindowDispatcherScheduler in several Universal Windows Platform applications, so far I confirm it works good without breaking anything, although those applications are all single window apps. This change seems reasonable to me, so I approve – at least this fixes the scheduler hijacking issue. Probably we should wait a bit longer to gather more opinions from UWP reviewers.

@glennawatson glennawatson merged commit 2fc4e6e into reactiveui:master Jul 1, 2019

2 checks passed

ReactiveUI-CI Build #9.18.12+4556512c76 succeeded
Details
license/cla All CLA requirements met.
Details

@hinterlandsupplyco hinterlandsupplyco deleted the hinterlandsupplyco:uwp-multiple-windows-bugs branch Jul 1, 2019

@glennawatson

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Thanks for this one @hinterlandsupplyco sure it will assist others.

@hinterlandsupplyco

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@glennawatson happy to contribute! :-)

madmonkey added a commit to madmonkey/ReactiveUI that referenced this pull request Jul 12, 2019

fix: Replace default `MainThreadScheduler` on UAP (reactiveui#2100)
See reactiveui#2092, reactiveui#2032 for more info. This replaces the usage of
`WaitForDispatcherScheduler` and `CoreDispatcherScheduler` with a
scheduler that explicitly dispatches to the main app window regardless
of how many windows are created by the application.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.