Make ImageSource more async-friendly

[symbiogenesis]

If SemaphoreSlim is used instead of locks, then the stream-based derived types of ImageSource can be more async-friendly.

These derived types are downloading image data from elsewhere, and that data is likely to take some significant time. Thus, the more async-friendly approach makes sense.

This seems most likely to help when the same ImageSource is being referenced in multiple places, which is the current purpose of the locks.

[MartyIX]

It would be great to describe the changes a bit more. Especially, what your goal is bere.

[symbiogenesis]

It would be great to describe the changes a bit more. Especially, what your goal is bere.

Ok, I will add a bit more.

[MartyIX]

btw: I guess a bit unrelated but I have noticed this line:

maui/src/Core/src/Platform/ImageSourcePartLoader.cs

Line 71 in 48d5ebf

.ConfigureAwait(false);

and I wonder why ConfigureAwait(false) is used there when

maui/src/Core/src/Platform/ImageSourcePartLoader.cs

Line 73 in 48d5ebf

SourceManager.CompleteLoad(result);

looks like it requires to be called by the UI thread rather than some random thread.

[symbiogenesis]

btw: I guess a bit unrelated but I have noticed this line:

maui/src/Core/src/Platform/ImageSourcePartLoader.cs

Line 71 in 48d5ebf

.ConfigureAwait(false);

and I wonder why ConfigureAwait(false) is used there when

maui/src/Core/src/Platform/ImageSourcePartLoader.cs

Line 73 in 48d5ebf

SourceManager.CompleteLoad(result);

looks like it requires to be called by the UI thread rather than some random thread.

seems to come from this PR #2352 by @PureWeen

My understanding is that ConfigureAwait(false) needs to be set all the way up the chain for it to be in effect. Any breaks in that, and it will revert to standard behavior.

Given that every time UpdateImageSourceAsync() is invoked in the codebase, it is not invoked with ConfigureAwait(false) that means that the setting has no effect within the codebase. Although, it is a public method and anyone could call it.

But, I suppose that having it set that way opens up the possibility for it to be called with ConfigureAwait(false) and have that work as expected. But if it is really needing the UI thread then probably doing that would not be desirable. Maybe there is some kind of headless test case where it makes sense.

[MartyIX]

My understanding is that ConfigureAwait(false) needs to be set all the way up the chain for it to be in effect. Any breaks in that, and it will revert to standard behavior.

That's not how async work. You can test the behavior by running this unit test:

[Fact]
public async Task TestAsync()
{
    System.Diagnostics.Debug.WriteLine($"{nameof(TestAsync)} [1] ThreadId={Environment.CurrentManagedThreadId}");

    await DeepAsync();

    System.Diagnostics.Debug.WriteLine($"{nameof(TestAsync)} [2] ThreadId={Environment.CurrentManagedThreadId}");
}

public async Task DeepAsync()
{
    System.Diagnostics.Debug.WriteLine($"{nameof(DeepAsync)} [1] ThreadId={Environment.CurrentManagedThreadId}");

    await DeeperAsync();

    System.Diagnostics.Debug.WriteLine($"{nameof(DeepAsync)} [2] ThreadId={Environment.CurrentManagedThreadId}");
}

public async Task DeeperAsync()
{
    System.Diagnostics.Debug.WriteLine($"{nameof(DeeperAsync)} [1] ThreadId={Environment.CurrentManagedThreadId}");

    await Task.Delay(1000).ConfigureAwait(false);

    System.Diagnostics.Debug.WriteLine($"{nameof(DeeperAsync)} [2] ThreadId={Environment.CurrentManagedThreadId}");
}

it prints:

TestAsync [1] ThreadId=16
DeepAsync [1] ThreadId=16
DeeperAsync [1] ThreadId=16
DeeperAsync [2] ThreadId=9 <-- This is what I'm talking about
DeepAsync [2] ThreadId=16
TestAsync [2] ThreadId=17

[symbiogenesis]

My understanding is that ConfigureAwait(false) needs to be set all the way up the chain for it to be in effect. Any breaks in that, and it will revert to standard behavior.

That's not how async work. You can test the behavior by running this unit test:

I like your unit test explanation. But, does SourceManager.CompleteLoad() really need the UI thread? It seems to just be calling Dispose() on a field, and two setters that have no logic.

public void CompleteLoad(IDisposable? result)
{
	_sourceResult = result;
	_sourceCancellation?.Dispose();
	_sourceCancellation = null;

	IsResolutionDependent = false;
	CurrentResolution = 1.0f;
}

[MartyIX]

The way I think about it is:

• Does it fail miserably? No.
• Is it correct? It seems it isn't because there is no synchronization (no locks, etc.)

So my best guess is that it works because it's not exercised too much and it's prone to introducing bugs in the future.

[symbiogenesis]

The way I think about it is:

• Does it fail miserably? No.
• Is it correct? It seems it isn't because there is no synchronization (no locks, etc.)

So my best guess is that it works because it's not exercised too much and it's prone to introducing bugs in the future.

It is possible there aren't enough locks in those other files, but I just checked and it appears that they are only instantiated within handlers for individual controls, and triggered on events like property mapping. So perhaps that is why the ImageSourcePartLoader and ImageSourceServiceResultManager do not currently have locks.

By contrast, ImageSource itself is something that you could bind to more than one bindable property.

[jsuarezruiz]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[rmarinho]

we don t use private

[symbiogenesis]

Fixed. Tests need to be rerun.

[jsuarezruiz]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).