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

InvalidOperationException when multiple UI threads are used #263

Closed
HeedfulCrayon opened this issue Sep 20, 2023 · 16 comments · Fixed by #266
Closed

InvalidOperationException when multiple UI threads are used #263

HeedfulCrayon opened this issue Sep 20, 2023 · 16 comments · Fixed by #266
Assignees
Labels
bug Something isn't working

Comments

@HeedfulCrayon
Copy link
Contributor

Describe the bug

When using multiple STA threads in an application (Main window that launches a child window in a new STA thread), the DialogService registers the views from multiple threads correctly. When accessing the views to find the owner, the application will crash with an InvalidOperationException stating "The calling thread cannot access this object because a different thread owns it"

To Reproduce

Steps to reproduce the behavior:

  1. You need a thread safe ServiceProvider that can give you access to the DialogService you registered across multiple threads (we are using the CommunityToolkit IoC).
  2. Create a Main window that registers a view with the DialogServiceViews.
  3. Create a new STA thread that launches a new window that also registers a view with the DialogServiceViews.
  4. Try to show the second view that was registered using the DialogService.
  5. Observe the crash

Expected behavior

The views should be registered in a threadsafe collection, and have checks so that the views that the DialogService is trying to find the owner for are only related to the current thread calling it.

Screenshots

Desktop (please complete the following information):

  • OS: Windows 10

Additional context

I have implemented a fix using a ConcurrentDictionary collection. I will reference this issue in that PR, but I would love any feedback you may have on it.

@HeedfulCrayon HeedfulCrayon added the bug Something isn't working label Sep 20, 2023
@github-actions
Copy link
Contributor

Hi there and welcome to this repository!

A maintainer will be with you shortly, but first and foremost I would like to thank you for taking the time to report this issue. Quality is of the highest priority for us, and we would never release anything with known defects. We aim to do our best but unfortunately you are here because you encountered something we didn't expect. Lets see if we can figure out what went wrong and provide a remedy for it.

@HeedfulCrayon
Copy link
Contributor Author

HeedfulCrayon commented Sep 21, 2023

Actually, the simplest course of action would be to make the IView interface and the ViewWrapper class and properties public. That way someone with special needs could implement their own DialogServiceViews class and DialogService class

@FantasticFiasco
Copy link
Owner

Hi and sorry for late response, I'll look into the problem.

@HeedfulCrayon
Copy link
Contributor Author

@FantasticFiasco if you are interested, I do have a working solution that is thread safe. I could create a PR for that if you want

@FantasticFiasco
Copy link
Owner

@HeedfulCrayon I think that would be better than adding changes to the public API. Do you have any code snippets we could add as tests making sure that this functionality doesn't regress with new versions?

@FantasticFiasco
Copy link
Owner

I'd happily collaborate with you on it. Tell me what you want to do and I'll do the other parts. We can work on a branch in your fork?

@HeedfulCrayon
Copy link
Contributor Author

@HeedfulCrayon I think that would be better than adding changes to the public API. Do you have any code snippets we could add as tests making sure that this functionality doesn't regress with new versions?

I did update the tests in my thread safe version, and all the tests seem to be passing. As for creating a multithreaded test to verify that it is working in a multithreaded scenario, I couldn't figure out how to do that. I will hopefully get a branch on my fork up in an hour or two

@FantasticFiasco
Copy link
Owner

FantasticFiasco commented Sep 27, 2023

Don't haste, I won't be able to make contributions for a day or so.

@HeedfulCrayon
Copy link
Contributor Author

Sorry, relatively new to open source collaboration. I forgot to remove some extra stuff I was using for debugging. My fork should demonstrate the fix. It also shouldn't break any existing unit tests

@FantasticFiasco
Copy link
Owner

FantasticFiasco commented Sep 28, 2023

np! I'll look into the changes. Do you have a small snippet or sample application that would demonstrate your setup with multiple STA threads? I'll look into whether we will test this using unit tests or automated UI tests.

@HeedfulCrayon
Copy link
Contributor Author

I can't really provide a snippet or sample, but the idea is this. Imagine a traditional MVVM application, and in the ViewModel you create a new thread, set it's apartment state to STA and that thread launches a new window. Now both windows have separate UI threads. The reason behind it is there is a Control Center that launches child windows. You can launch multiple child windows that should all have their own UI threads.

@HeedfulCrayon
Copy link
Contributor Author

I have an idea for testing the threading aspect. I will have it up soon

HeedfulCrayon pushed a commit to HeedfulCrayon/mvvm-dialogs that referenced this issue Sep 28, 2023
HeedfulCrayon pushed a commit to HeedfulCrayon/mvvm-dialogs that referenced this issue Sep 28, 2023
@FantasticFiasco
Copy link
Owner

I like your test! I'm looking into building a sample showcasing the support for different STA threads.

@FantasticFiasco
Copy link
Owner

Could you please open up a new pull request from the bug/FixInvalidOperationExceptionWithMultiThreading branch, and check the option to allow me to push changes to this branch? Thanks!

@HeedfulCrayon
Copy link
Contributor Author

Looks like there is one test failure. I will see if I can solve that issue

@HeedfulCrayon
Copy link
Contributor Author

Got it fixed. I needed to use the View's HashCode instead of the View Id since the ViewWrapper generates a new Id every time

FantasticFiasco added a commit that referenced this issue Oct 7, 2023
Fixes #263

---------

Co-authored-by: i28423 <[email protected]>
Co-authored-by: FantasticFiasco <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants