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

Connectivity implementation not thread-safe on Windows, returning incorrect network status #9972

Closed
breenbob opened this issue Sep 7, 2022 · 11 comments · Fixed by #10062
Closed
Labels
area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info fixed-in-7.0.0-rc.2.6866 Look for this fix in 7.0.0-rc.2.6866! platform/windows 🪟 t/bug Something isn't working

Comments

@breenbob
Copy link
Contributor

breenbob commented Sep 7, 2022

Description

On the WinUI platform, any calls to Connectivity.Current only work from the first thread to access the property. Subsequent calls from any other thread will cause a silent thread access exception:
image

That will end in a value of Unknown being returned:
image

Doesn't matter which thread is first - main thread or background thread, it will always return correct value, and continue to return the correct value for that thread. But if you try to access it from a different thread, that is when this issue occurs.

Steps to Reproduce

I've linked to a sample repro below. Just run and click button 1 then button 2, or button 2 then button 1 to invert the outcome.

Main thread before bg thread:
xJ9MaGocEK

Bg thread before main thread:
TSWdWFLxeB

Link to public reproduction project repository

https://github.com/breenbob/MauiWindowsConnectivityIssue

Version with bug

6.0.400

Last version that worked well

Unknown/Other

Affected platforms

Windows

Affected platform versions

10.0.17763.0

Did you find any workaround?

If checks are always done on the same thread you won't see the issue, but that's not a feasible long term solution.

Relevant log output

No response

@breenbob breenbob added the t/bug Something isn't working label Sep 7, 2022
@PureWeen PureWeen added the area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info label Sep 7, 2022
@PureWeen PureWeen added this to the Backlog milestone Sep 7, 2022
@ghost
Copy link

ghost commented Sep 7, 2022

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.

@breenbob
Copy link
Contributor Author

breenbob commented Sep 7, 2022

Not quite a duplicate of but probably same underlying cause as #7393

@MagicAndre1981
Copy link
Contributor

using Device.BeginInvokeMainThread() should always work

@breenbob
Copy link
Contributor Author

breenbob commented Sep 8, 2022

@MagicAndre1981 yes as I said in the workarounds if you consistently access from same thread it's not an issue, but it is not always possible/practical to do that, and it is thread safe on the other platforms. Also, in .Net Maui you need to use MainThread.BeginInvokeOnMainThread(), Dispatcher.Dispatch() or App.Current.Dispatcher.Dispatch() to access the UI thread (depending on your scenario, e.g. multi-window where each window has it's own UI thread) as Device.BeginInvokeOnMainThread() is obsolete.

@BenBtg
Copy link
Contributor

BenBtg commented Sep 8, 2022

I've been working with a customer who is also seeing this issue.

@janseris
Copy link

janseris commented Sep 9, 2022

FYI that was the point I gave up Windows MAUI development. After many disappointments, this was the final one, breaking the app.
As you can see 3.5 months and nothing has changed. (referring to #7393)

@MagicAndre1981
Copy link
Contributor

ok, this will be fixed with #10062 and the fixed code also calls it on Main Thread

@breenbob
Copy link
Contributor Author

Great stuff, thanks @jsuarezruiz and @BenBtg!

@breenbob
Copy link
Contributor Author

@jsuarezruiz Does this also fix #7393?

@janseris
Copy link

Why does Internet check have to run on UI thread anyways?

@ghost ghost locked as resolved and limited conversation to collaborators Oct 13, 2022
@mattleibow
Copy link
Member

I am super not sure about things. I am reverting the main thread "fix" as this is breaking other things: microsoft/WindowsAppSDK#2965 (comment)

But I cannot repro the original issue anymore nor can I repro the issue in this microsoft/WindowsAppSDK#2965 (comment)

I think this may have been a windows app sdk issue and they fixed it - or maybe even an OS issue. I had an issue with XXH1 and it was fixed in XXH2. (can't recall offhand anymore, but yeah... OS was dodgy)

@samhouts samhouts added the fixed-in-7.0.0-rc.2.6866 Look for this fix in 7.0.0-rc.2.6866! label Feb 17, 2023
@samhouts samhouts modified the milestones: Backlog, .NET 8 May 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info fixed-in-7.0.0-rc.2.6866 Look for this fix in 7.0.0-rc.2.6866! platform/windows 🪟 t/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants