maui

maui copied to clipboard

Description

Calling subscribe and unsubscribe operations on one thread while doing MessagingCenter calls on another can cause a variety of multi-threading exceptions. The _subscriptions dictionary and the normal list can be changed on one thread while another thread is doing a non thread safe operation like any(), contains(), etc.

The comment on line 207 of the MessagingCenter.cs class acknowledges this issue but the fix used is not thread safe either; the ToList() method itself could fail due to it.

This bug has also been entered under the Xamarin Forms repo as it exists there as well: https://github.com/xamarin/Xamarin.Forms/issues/15563

Steps to Reproduce

• Get and run solution at https://github.com/Bowman74/MAUIMessagingCenterBug
• Press the "No Errors" button and watch the debug window. No errors will happen. More on this later, but this is due to using a SemaphoreSlim to protect multithreaded access To the Subscribe and Unsubscribe methods. When done close the popup window to allow the background tasks to complete.
• Press the "Create Error" button and watch the debug window. Multiple errors will happen. This method is not doing anything to protect the subscribe and unsubscribe methods and just calls into MessagingCenter directly. When done close the popup window to allow the background tasks to complete.
• Even worse. Without closing the app after the "Create Error" issue, press the "No Errors" button and watch the debug window. You will now observe multiple errors where there were none before. This is because the exceptions caused in the prior step left the MessagingCenter's _subscriptions dictionary and it's contained list's in an unknown state. Until the app is restarted, MessagingCenters will experience problems when new subscriptions added or removed.

Expected Behavior Methods on the MessagingCenter can be called on multiple threads without issue

Actual Behavior Subscribing and unsubscribing on multiple threads at the same time can cause a variety of exceptions (normally System.InvalidOperationException). If these exceptions are caught and not allowed to crash the app, MessageCenter can be left in an invalid state and not be usable until the app is restarted.

Link to public reproduction project repository

https://github.com/Bowman74/MAUIMessagingCenterBug

Version with bug

6.0.486 (current)

Last version that worked well

Unknown/Other

Affected platforms

Android

Affected platform versions

Android 31

Did you find any workaround?

Wrapping all the methods of the MessagingCenter with containment and delegation and use a SemaphoreSlim to ensure only one thread can access it at at time will correct the issue and is likely the correct solution for. The repository shows a class that fixes the problem using this approach.

This bug likely impacts all platforms.

Relevant log output

No response

MessagingCenter is mostly still here for compatibility reasons. Also see #3880 and #8765. You will probably want to use a replacement. The one in the .NET Community Toolkit is very fast and should be compatible with .NET MAUI.

@jfversluis That's fair. However, it isn't under a compatibility namespace or documentation so MAUI users are not going to know that. It currently appears to be a first class part of MAUI. A non code fix would be to change the documentation to state that it is here for compatibility, point to the community toolkit and mention that the current implementation isn't thread safe. People coming from XF are likely going to use it by default, particularly given its namespace unless you add the obsolete attribute.

https://learn.microsoft.com/en-us/dotnet/api/microsoft.maui.controls.messagingcenter?view=net-maui-6.0

Absolutely agree. As you can see in the linked PRs we were working on that, but somehow we didn't get to it yet. But I agree that it should be more clear and while it isn't we might still just need to fix this now since it's officially still supported. Was just providing you with some info and an alternative, that's also why I left it open :)

@jfversluis I absolutely appreciate the information. 👍

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

Just use events no need to reinvent the wheel

@janseris Events are not the same thing at all. Events require strong coupling and the subscriber to know about and have a reference to the publisher.

Messaging services like MessagingCenter decouple the application. A publisher will send out a message to the aether, "I don't know who needs to know this, but this thing just happened." There could be nothing that is listening for that or ten things listening for it. The sender has no idea who is listening for the messages and the listeners have no idea who (if anyone) is sending them.

When using events on disconnected parts of the application, they couple them up and make the application very brittle. Events make sense between parts of an architecture that already have knowledge of the other like a parent and a child or a view and its view model but make little sense on parts of the application that should otherwise have no knowledge of each other.

I don't think we're going to fix this anymore. The MessagingCenter has already been deprecated and will be removed for .NET 8. I would suggest to use the CommunityToolkit MVVM Messenger: https://learn.microsoft.com/dotnet/communitytoolkit/mvvm/messenger

Thanks everyone for the input and special thanks to @Bowman74 for the extensive investigation here!