Fix possible freeze when downloading avatars

[lanfeust69]

Fixes #5859

[vbjay]

using (var imageStream =  await readTask)
		{
			using (var img = System.Drawing.Image.FromStream(imageStream)
			{
				return (Image)img.Clone();
			}
		}

https://docs.microsoft.com/en-us/dotnet/api/system.drawing.image.fromstream?view=netframework-4.7.2#System_Drawing_Image_FromStream_System_IO_Stream_

Remarks
You must keep the stream open for the lifetime of the Image.

[lanfeust69]

This is unrelated to the fix, isn't it ?

I'm surprised by this piece of documentation, by the way : it is also stated for the overload Image.FromStream(Stream, bool) but not for the "main" overload Image.FromStream(Stream, bool, bool), to which the others redirect. It's hard to tell for sure since the implementation delegates quickly to something native, but my guess is that the stream is actually read during the call, and the documentation is incorrect.

Also note that there is another place where Image.FromStream is called with the same pattern, within a using block.

[vbjay]

It creates a GDI+ handle. Since the docs say it and the code here. Since we don't know how GDI+ works and how cleanup works in GDI+ and they did mention it in the docs, I don't see a problem of using clone to make sure. The fact you were touching the code made me see the issue. Yes we will need to visit all image.fromstream calls.

[vbjay]

Also this is where cleanup occurs. So my using on the image does cleanup the image from the stream and we have an image that we don't have to worry about the stream closing mess.

Dispose() passes true into Dispose(bool).

[vbjay]

This is why:

GDI+, and therefore the System.Drawing namespace, may defer the decoding of raw image bits until the bits are required by the image. Additionally, even after the image has been decoded, GDI+ may determine that it is more efficient to discard the memory for a large Bitmap and to re-decode later. Therefore, GDI+ must have access to the source bits for the image for the life of the Bitmap or the Image object.

[vbjay]

Doing more research. Looks like clone might not be enough. https://stackoverflow.com/questions/12709360/whats-the-difference-between-bitmap-clone-and-new-bitmapbitmap/13935966#13935966

[lanfeust69]

Anyway, assuming something has to be done (not 100% clear : this pattern exists as it is since at least 8d27233, 8 years ago, apparently without actual issue), it should be in a separate PR, shouldn't it ?

[vbjay]

Alright. We will get it from the horse's mouth. Then we can raise an issue to fix it based on their feedback.

[RussKie]

Anyway, assuming something has to be done (not 100% clear : this pattern exists as it is since at least 8d27233, 8 years ago, apparently without actual issue), it should be in a separate PR, shouldn't it ?

Yes, I agree, this PR is about blocking UI.

[drewnoakes]

This will unblock the UI thread, but it will also potentially lock an arbitrary number of thread pool threads. If the user loads a repo and scrolls through commits requiring a large number of uncached user avatars, the thread pool will be exhausted and the application will freeze.

A robust solution needs to limit the number of concurrent requests.

[RussKie]

I'm not near a computer, but are we addressing the issue at the right place? The code in question is async, perhaps the call site needs to be updated to use the JTF to unblock the UI thread?

…

On Mon, Dec 10, 2018, 8:08 PM Drew Noakes ***@***.***> wrote: ***@***.**** commented on this pull request. This will unblock the UI thread, but it will also potentially lock an arbitrary number of thread pool threads. If the user loads a repo and scrolls through commits requiring a large number of uncached user avatars, the thread pool will be exhausted and the application will freeze. A robust solution needs to limit the number of concurrent requests. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#5868 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEMyXlFbA_K0sprWAoeSIEayXWuvZiZ3ks5u3iSggaJpZM4ZJmk2> .

[lanfeust69]

@RussKie : The problem is that the code is not really async : it blocks synchronously (before returning the Task), even though the calling site does use JTF (if my understanding is correct).

@drewnoakes : Yes, this is possible, at least in theory. I'm not sure how likely it is in practice, though. OTOH, this is a workaround for something that is essentially a bug in .Net, I think keeping things as minimal as possible has some appeal. I'll try to see if I can address this, but I can't tell when.

[drewnoakes]

The problem is that the code is not really async

Agreed.

BTW I would rather see await TaskScheduler.Default to get onto a thread pool thread than Task.Run.

A robust solution needs to limit the number of concurrent requests.

Yes, this is possible, at least in theory. I'm not sure how likely it is in practice

Depending upon the machine, you can hit thread pool exhaustion at 25 threads. We do a lot of background work at startup too. I would not brush this off as theoretical.

I think keeping things as minimal as possible has some appeal

Adding such protection can be minimal. Use a SemaphoreSlim created with a limit (maybe 10) and then WaitAsync and Release accordingly.

The code would end up looking something like:

private readonly SemaphoreSlim _downloadSemaphore = new SemaphoreSlim(initialCount: 10);

And later:

using (var webClient = new WebClient { Proxy = WebRequest.DefaultWebProxy })
{
    webClient.Proxy.Credentials = CredentialCache.DefaultCredentials;

    // WebClient.OpenReadTaskAsync can block before returning a task, so make sure we
    // make such calls on the thread pool, and limit the number of concurrent requests.

    // Get onto background thread
    await TaskScheduler.Default;

    // Limit the number of concurrent download requests
    await _downloadSemaphore.WaitAsync();

    try
    {
        using (var imageStream = await webClient.OpenReadTaskAsync(imageUrl))
        {
            return Image.FromStream(imageStream);
        }
    }
    finally
    {
        _downloadSemaphore.Release();
    }
}

That's completely untested, but gets the idea across.

[lanfeust69]

@drewnoakes : That's indeed the kind of stuff (WaitAsync on a semaphore) I had in mind when I was saying "I'll try to see if I can address this, but I can't tell when." But still not sure when I can really look into it 😃 ...

[drewnoakes]

No problem. I'll put a PR together.

[drewnoakes]

@lanfeust69 please review #5878. Thanks.

[RussKie]

Superseded by #5878