Image.LoadAsync(stream) uses synchronous IO if the stream is seekable

[kroymann]

ImageSharp version

1.0.4 (verified issue is still present in 2.0.0)

Other ImageSharp packages and versions

ImageSharp.Web 1.0.5 (+ fixes from PR 222)

Environment (Operating system, version and so on)

Azure App Service (Windows-based)

.NET Framework version

6.0

Description

Image.LoadAsync(stream) ends up synchronously reading from the source stream in both Image.InternalDetectFormatAsync() and then again in ImageDecoderUtilities.DecodeAsync(). When running in a webservice under high-load scenarios, this can quickly lead to thread pool exhaustion as all threads are blocked waiting on network IO.

private static Task<IImageFormat> InternalDetectFormatAsync(Stream stream, Configuration config)
{
    return Task.FromResult(InternalDetectFormat(stream, config));
}

public static Task<Image<TPixel>> DecodeAsync<TPixel>(...)
{
    using var bufferedReadStream = new BufferedReadStream(configuration, stream);
    Image<TPixel> image = decoder.Decode<TPixel>(bufferedReadStream, cancellationToken);
    return Task.FromResult(image);
}

Steps to Reproduce

Try to load an image asynchronously and observe that threads end up blocked on synchronous reads

Images

No response

[kroymann]

I initially started to implement a PR that would fix this by making both of those methods actually async, but that has long tendrils that extend deep into the image decoding code. And even if you ignore the amount of duplicate sync/async code paths such a PR would create, I'm now questioning whether that's actually even the right way to fix this.

As a bit of background, this issue only occurs if the source stream passed in to Image.LoadAsync() is seekable, because in that scenario the image decoding reads directly from the source stream. If the source stream is not seekable, then the code first creates a new ChunkedMemoryStream, then asynchronously copies the source data into that in-memory stream, and finally performs the rest of the actions on that in-memory stream. In that latter scenario, synchronous reads are not only fine, but are actually preferrable as they avoid unnecessary overhead.

After thinking about this a bit, I'm now of the opinion that Image.WithSeekableStreamAsync() should always copy the image to a ChunkedMemoryStream, and then the rest of the internal image decoding logic should use explicitly synchronous code paths, removing any facade of being async at all. I don't have hard data to back this up, but I suspect that isolating the asynchronous network IO reads to Stream.CopyToAsync() would yield better performance than sprinkling those reads throughout the image decoding flow.

NOTE: For the synchronous path, Image.WithSeekableStream() could still keep the optimization that skips creating an in-memory stream if the source is seekable. It would just be in the async path that the code would always copy into an in-memory stream.

[kroymann]

The super simple change here is to just remove the if (stream.CanSeek) { ... } block from Image.WithSeekableStreamAsync().

A more involved change would be to also remove all of the async internal methods and exclusively use synchronous methods since we know that at this point we're always reading from our own in-memory stream. That would include all of these private methods:

• Image.InternalDetectFormatAsync()
• Image.InternalIdentityAsync()
• Image.DecodeAsync()
• Image.DiscoverDecoderAsync()

And more significantly, this public interface method:

• IImageDecoder.DecodeAsync()

We would also want to change the IImageDecoder.Decode() method so that it takes a CancellationToken in order to plumb that token through to the IImageDecoderInternals.Decode() method (currently the synchronous image decode code path doesn't provide the ability to pass in a CancellationToken, even though the internal code is built to support it).

[antonfirsov]

When running in a webservice under high-load scenarios, this can quickly lead to thread pool exhaustion as all threads are blocked waiting on network IO.

NetworkStream is not seekable and I would assume file IO is much faster, so I wonder what is happening in your scenario. Do you have more details / data?

[kroymann]

Unfortunately, yes it does. Over that last few weeks our production service has been encountering sporadic outages that we've tracked down to thread pool exhaustion caused by these reads. The Azure Blob Storage SDK appears to be returning streams that are seekable:
https://github.com/Azure/azure-storage-net/blob/b0d47bfd4c1c0dcbe60eaf373e2977cd7405aa5f/Lib/Common/Blob/BlobReadStreamBase.cs#L94

[kroymann]

I just reviewed the version of the Azure SDK being used by the ImageSharp.Web.Providers.Azure library and confirmed that it too returns a Stream that is seekable:
https://github.com/Azure/azure-sdk-for-net/blob/f31090ad65df180fb8b8d07a263ea0fe0c558869/sdk/storage/Azure.Storage.Common/src/Shared/LazyLoadingReadOnlyStream.cs#L355

[antonfirsov]

Thanks!

There is no easy solution. Updating decoders to work in a truly async manner is very hard problem as you figured out, I don't think we'll ever go down that rabbit hole. Always prefetching into a ChunkedMemoryStream sounds reasonable but that would increase the memory footprint and may put a lot of pressure on ArrayPool<byte>.Shared. Probably still the lesser evil. I wonder if we should make it configurable through an option on Configuration?

[kroymann]

From my vantage point, having an API that advertises itself as being async, but which internally uses synchronous IO seems like a pretty nasty anti-pattern that can't be viewed as anything other than a bug. All it takes is a short hiccup in network throughput and you can quickly consume every thread in the pool with blocking IO.

Given that the decoders aren't actually async, and as you said it's highly unlikely that anyone will tackle that work, I only see two valid paths to take here:

1. Have the async APIs always copy into an in-memory buffer so that the decoder can then safely execute without incurring blocking IO (and then advertise this behavior as broadly as possible via comments, documentation, etc).
2. Remove the async APIs entirely and instead maybe provide a simple demonstration in the docs of how to use something like a RecyclableMemoryStreamManager to asynchronously download an image into memory before calling Image.Load():

using (var memoryStream = recyclableMemoryStreamManager.GetStream())
{
    await sourceStream.CopyToAsync(memoryStream);
    memoryStream.Position = 0;
    return Image.Load<Rgba32>(configuration, memoryStream);
}

As it stands right now, we're already in a state where the API is effectively synchronous anyway, so going with the second option isn't really a big loss in functionality - it's merely acknowledging the reality that the image loading code doesn't actually have an async path, as opposed to creating the illusion that the library supports async image loading.

[kroymann]

By the way I want to make sure my comments aren't coming off as angry/aggressive/demanding. I am extremely appreciative of the work that all of the maintainers do on this fantastic product. 🙇 I just want to help ensure that other users of this library don't get bitten by this same issue where they mistakenly believe that LoadAsync() will use async IO to read the image.

[antonfirsov]

As it stands right now, we're already in a state where the API is effectively synchronous anyway, so going with the second option isn't really a big loss in functionality - it's merely acknowledging the reality that the image loading code doesn't actually have an async path

This is a very hard breaking change, basically no-go. We should aim to find a fix instead. We should go ahead and prototype a unconditional ChunkedMemoryStream preloading. The hardest part of the job is async-enabling LoadResizeMemoryStress, and comparing the memory pressure with/without preloading to make sure we don't regress significantly.

@kroymann are you interested in contribution? If yes, I can provide more guidance.

@JimBobSquarePants any thoughts? Are you fine with such a change in principle, if it doesn't regress us in other means?

[kroymann]

@antonfirsov Happy to contribute (I was already deep down the path of implementing a PR when I filed this issue). I've been contributing on the ImageSharp.Web project periodically over the years but haven't had a reason until now to dig into ImageSharp itself.

That said, I think I'd still like to make a pitch in favor of the hard breaking change as it feels like a cleaner acknowledgement of the true reality of the code. The core ImageSharp library simply does not have support for asynchronous image loading, and the existing async APIs with their Task.FromResult() wrappers around their synchronous counterparts are just misleading people into thinking that it does. Rather than bake in a hidden async preload followed by synchronous load, as a consumer I personally would prefer for the library to make it crystal clear to me that it does not support asynchronous loading at all, and then let me add my own async preloading before calling into ImageSharp if that's what I want to do.

I am curious to hear what @JimBobSquarePants thinks on the topic though.

[antonfirsov]

The core ImageSharp library simply does not have support for asynchronous image loading

This is not true for the path where the stream is pre-fetched to ChunkedMemoryStream. It's sub-optimal, but truly async, since there is no blocking IO call.

and then let me add my own async preloading before calling into ImageSharp if that's what I want to do

The library optimizes this scenario with pooled allocations within ChunkedMemoryStream. MemoryStream allocates a single large GC array, which creates a lot of GC pressure and hurts throughput.

[kroymann]

The library optimizes this scenario with pooled allocations within ChunkedMemoryStream. MemoryStream allocates a single large GC array, which creates a lot of GC pressure and hurts throughput.

That's definitely true. In my product's code, after discovering this problem, we're now working around it by preloading the stream into a RecyclableMemoryStream which does similar chunking.

[JimBobSquarePants]

There's only one option and that's to do the copy. We simply cannot make such a the breaking change.

To reduce ArrayPool pressure I would replace our own ChunkedMemoryStream with RecyclableMemoryStream. The only reason I didn't use it originally was because we were targeting netstandard 1.3

We can then expose the RecyclableMemoryStreamManager via Configuration as we do in ImageSharp.Web ImageSharpMiddlewareOptions

https://github.com/SixLabors/ImageSharp.Web/blob/6912cbc719b6fbab16ff7c8c86e0567e57e0c7c7/src/ImageSharp.Web/Middleware/ImageSharpMiddlewareOptions.cs#L55-L59

I don't think we should make the copy optional and always default to using it.

@kroymann If you could go ahead and give that a go that would be ace! ❤️

[antonfirsov]

RecyclableMemoryStream would bring an external dependency and create a second parallel set of memory pools. I'd prefer to avoid this in the core library.

By ArrayPool pressure I particularly mean the problem of exhausting a single pool bucket, since ChunkedMemoryStream rents uniform buffers. We can try to address this it by incrementally growing ChunkedMemoryStream's chunkLength as the stream grows, which is RecyclableMemoryStream's primary trick.