Skip to content

Make ImageSource more async-friendly #22098

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

Merged
merged 1 commit into from
Feb 1, 2025

Conversation

symbiogenesis
Copy link
Contributor

@symbiogenesis symbiogenesis commented Apr 27, 2024

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.

@symbiogenesis symbiogenesis requested a review from a team as a code owner April 27, 2024 15:07
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Apr 27, 2024
@MartyIX
Copy link
Contributor

MartyIX commented Apr 27, 2024

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

@symbiogenesis
Copy link
Contributor Author

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
Copy link
Contributor

MartyIX commented Apr 27, 2024

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

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

SourceManager.CompleteLoad(result);

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

@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Apr 27, 2024

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

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

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
Copy link
Contributor

MartyIX commented Apr 28, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

MartyIX commented Apr 28, 2024

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
Copy link
Contributor Author

symbiogenesis commented Apr 28, 2024

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
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz jsuarezruiz added area-image Image loading, sources, caching area-controls-image Image control labels Apr 29, 2024
@dotnet-policy-service dotnet-policy-service bot added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Apr 29, 2024
@rmarinho rmarinho requested a review from mattleibow May 6, 2024 11:40
@symbiogenesis symbiogenesis force-pushed the imagesource-semaphore branch from 3a05266 to c97bbe5 Compare May 6, 2024 20:21
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Eilon Eilon removed legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor area-image Image loading, sources, caching labels May 10, 2024
@mattleibow
Copy link
Member

/rebase

@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@github-actions github-actions bot force-pushed the imagesource-semaphore branch from c97bbe5 to 69d3694 Compare January 28, 2025 13:51
@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks all good. I rebased so we shall see what the machines say. We got many more tests at this point, so I am sure if they all pass then things are great.

It is internal code, so no user code is going to change.

@mattleibow mattleibow added this to the .NET 9 SR5 milestone Jan 28, 2025
@mattleibow mattleibow self-assigned this Jan 28, 2025
@mattleibow mattleibow modified the milestones: .NET 9 SR5, .NET 9 SR4 Jan 28, 2025
@PureWeen PureWeen merged commit a026e74 into dotnet:main Feb 1, 2025
316 checks passed
@symbiogenesis symbiogenesis deleted the imagesource-semaphore branch February 1, 2025 18:13
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-image Image control community ✨ Community Contribution
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants