dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.22k stars 1.75k forks source link

[Android] Cannot retrieve data from SecureStorage #18685

Open leonluc-dev opened 1 year ago

leonluc-dev commented 1 year ago

Description

I'm using the Xamarin/Maui Essentials SecureStorage in my iOS/Android project. Recently I started migration from Xamarin to Maui and I am running into a issue.

Whenever I store a large string (2500 characters in this example) in secure storage using SecureStorage.SetAsync() it seems to work fine (the returned task completes with no errors), but as soon as I try to retrieve the string using SecureStorage.GetAsync() the returned task hangs and never completes. If I try to retrieve shorter strings this doesn't occur and the GetAsync returns properly Edit: This doesn't seem to be related to larger strings but might be caused by synchronously waiting for GetAsync() (either directly or further up the call stack).

This issue only occurs on Android. On iOS/Mac/Windows this all works fine. This also isn't an issue in Xamarin.Android/Xamarin.Forms.

It's reproducible in both net7.0-android and net8.0-android (not tested on net6.0-android).

Steps to Reproduce

  1. Create a MAUI-based Android app with Maui or Maui Essentials enabled
  2. In an async method, store data in securestorage using await SecureStorage.Default.SetAsync("my_example_string", largeString);
  3. Try to retrieve the string using var retrievedValue = await SecureStorage.Default.GetAsync("my_example_string");
  4. Wait for the calling method using GetAwaiter().GetResult()
  5. The SecureStorage.GetAsync() task/await never completes

Link to public reproduction project repository

https://github.com/leonluc-dev/SecureStorageIssueReproduce

Version with bug

Tested and reproduced on: 7.0.101 8.0.3 8.0.3-dev (commit 71687020392deca465f051bc808906836068cc39 built from source)

Is this a regression from previous behavior?

Yes, this used to work in Xamarin.Essentials

Last version that worked well

Unknown/Other

Affected platforms

Android

Affected platform versions

Android 6, 7, 11, 12 and 13 (probably more)

Did you find any workaround?

None so far

Relevant log output

No response

jsuarezruiz commented 12 months ago

I think this issue will be fixed by https://github.com/dotnet/maui/pull/17928 in the next release.

ghost commented 12 months ago

We've added this issue to our backlog, and we will work to address it as time and resources allow. If you have any additional information or questions about this issue, please leave a comment. For additional info about issue management, please read our Triage Process.

leonluc-dev commented 12 months ago

@jsuarezruiz

I think this issue will be fixed by #17928 in the next release.

I built the maui sdk from source (main branch) to test this fix. While it seems to work again for smaller projects, it still hangs in the same way on a larger codebase. I'm currently trying to pinpoint what exactly causes the issue to still appear in some of my larger projects.

leonluc-dev commented 12 months ago

After a lot of debugging, it seems it's not specifically related to the size of the data.

I've managed to create a minimal reproduction of the issue here (using .NET Android with MAUI Essentials): https://github.com/leonluc-dev/SecureStorageIssueReproduce

In the example when the ApiSession object is first accessed (by the MainActivity's OnCreate in this case) it tries to restore its arbitrary sessiondata from the securestorage during initialization. This fails due to the GetAsync never returning.

I'm aware a GetAwaiter().GetResult() isn't the best idea (but alas, legacy code) but this worked fine in Xamarin.Essentials (and MAUI essentials on iOS/Mac)

Note: <PackageReference Include="Microsoft.Maui.Essentials" Version="8.0.3-dev" /> is a reference to a local version of the Essentials package built from commit 71687020392deca465f051bc808906836068cc39. This commit should already include the fix #17928 mentioned earlier, yet I can still reproduce the hanging GetAsync on Android.

sergiiPu commented 8 months ago

the issue is not fixed even in 8.0.7

ChristopherStephan commented 7 months ago

Reproducible with Microsoft.Maui.Essentials v8.0.10.

jonpryor commented 7 months ago

It is extremely important to note that one of the fundamental changes between Xamarin (.Android/.iOS/.Forms) and .NET (Android/iOS/MAUI) is that the BCL implementation changed from Mono (parts of which were based on open-sourced .NET Framework, parts of which are re-implementations) to CoreCLR. This change has lots of compatibility implications, from the "obvious" (AppDomain.CreateDomain() throws an exception on .NET) to the "subtle" (Behavior changes when comparing strings on .NET 5+).

It would not surprise me at all that Task behavior is one of the areas impacted.

Furthermore, there is quite a lot of advice and design guidelines around how to use async/await, including Asynchronous programming scenarios, which states:

  • Write code that awaits Tasks in a non-blocking manner Blocking the current thread as a means to wait for a Task to complete can result in deadlocks and blocked context threads and can require more complex error-handling. …
Use this… Instead of this… When wishing to do this…
await Task.Wait or Task.Result Retrieving the result of a background task

Using .GetAwaiter().GetResult() is (more or less) equivalent to using the Task.Result property, and has the same potential deadlock issues.

The only reliable fix is to update your code so that async/await is properly used. Using Task.Result or equivalents directly is courting deadlocks. Just because your code works "now" without deadlocks does not mean it will continue to do so, and the Xamarin to .NET migration is a perfect example of that.

leonluc-dev commented 7 months ago

I'm aware the only way to reliably fix this deadlock is to properly implement async/await. Problem is this is legacy code any many many pieces of other legacy code access it synchronously.

For now a workaround I found is to execute any code accessing the SecureStorage methods on Android with Task.Run (explicitly moving the execution to the threadpool). For example:

public Task<string> GetMySecuredString(string key)
{
    return Task.Run(async delegate 
    {
        return await SecureStorage.GetAsync(key);
    });
}

The GetMySecuredString method can now be called using GetAwaiter().GetResult()

MartinLichtblau commented 6 months ago

A SecureStorage.GetAsync() call in our Android app takes ~300ms. Problem is that we have to check SecureStorage for every config key lookup. Since ConfigurationProvider.TryGet() is not async we cannot use await Task.Run and face the risk that our app gets stuck, if SecuresStorage never returns a result ‒ certainly it slows things down.

thomasgalliker commented 6 months ago

I was struggling with exactly the same problem. Task.Run didn't help in my case. The platform-specific implementation of SecureStorage on Android does the same already. Have a look a the source code.

The problem: In my csproj file, I had <EmbedAssembliesIntoApk>true</EmbedAssembliesIntoApk> for Android debug builds. This seems to remove all the files written in the app directory whenever I start a new debug session.

The solution: Enable "fast assembly deployment" --> Set EmbedAssembliesIntoApk=false in the .csproj file for Android debug builds. You can still use EmbedAssembliesIntoApk=true for Android release builds. Have a look at this sample app.

It would be great to have a Microsoft recommendation for Android/Debug+Release and iOS/Debug+Release build configurations, something like "best practices".