Make async methods cancellable

[pekspro]

I think it would be nice if all a Load and Save sync methods could be cancellable. In principle it should be possible to add CancellationToken cancellationToken = default to all public method and then pass on the token to all methods all the way to stream object when reads and writes occurs.

I think this would be fairly easy to do. It just requires a lot of tiny changes, but I am willing to do the work :)

[dlemstra]

Sounds like a good plan :) Feel free to open up a PR for this.

[antonfirsov]

I disagree it's that simple. See my comment below.

[antonfirsov]

@pekspro the problem is that these are fake async methods. This is what is what usually happens in case of decoding:

1. The input Stream is copied to a MemoryStream in an asynchronous way
2. The synchronous, non-cancellable decoder logic is executed on the MemoryStream.

Step 2 takes significantly longer execute than step 1, and the logic is much more complicated (+ it's per decoder+encoder, and we support 5 formats ATM). I don't see value for doing it only for step 1, it would be a false promise to users. I think this should happen together with the introduction of first real async codec implementations.

@JimBobSquarePants @tocsoft what is most interesting in this proposal is that we probably need to introduce a breaking change on our decoder API before 1.0 (!) to make this possible in the future:

public interface IImageDecoder
{
-    Task<Image<TPixel>> DecodeAsync<TPixel>(Configuration configuration, Stream stream) where TPixel : unmanaged, IPixel<TPixel>;
-    Task<Image> DecodeAsync(Configuration configuration, Stream stream);
+    Task<Image<TPixel>> DecodeAsync<TPixel>(Configuration configuration, Stream stream, CancellationToken cancellationToken) where TPixel : unmanaged, IPixel<TPixel>;
+    Task<Image> DecodeAsync(Configuration configuration, Stream stream, CancellationToken cancellationToken);
}

[JimBobSquarePants]

The input Stream is copied to a MemoryStream in an asynchronous way

To further complicate matters that input stream is also only copied if the original is not seekable. It's all sleight of hand.

ImageSharp/src/ImageSharp/Image.FromStream.cs

Lines 705 to 721 in c0594f7

	if (stream.CanSeek)
	{
	if (configuration.ReadOrigin == ReadOrigin.Begin)
	{
	stream.Position = 0;
	}
	return await action(stream).ConfigureAwait(false);
	}
	using (MemoryStream memoryStream = configuration.MemoryAllocator.AllocateFixedCapacityMemoryStream(stream.Length))
	{
	await stream.CopyToAsync(memoryStream).ConfigureAwait(false);
	memoryStream.Position = 0;
	return await action(memoryStream).ConfigureAwait(false);
	}

According to #926 (comment) we still haven't managed to solve the loading issue we've been having with Azure streams anyway.

[tocsoft]

I feel we should just add the cancellation tokens in now, I would say it was just an oversight that it wasn't there in the first place. Like with all our async stuff it more about the API shape not the implementation that's important at this stage, and it would be relatively trivial to pipe the token throughout the sync decoder code and bomb out if its ever cancelled.

[antonfirsov]

We can also consider changing the sync decode/encode interface methods to accept a CancellationToken. A bit nasty but wouldn't be a revolutionary idea, sort-of lesser evil. Would be easy to implement in existing codec code. Real async support is way too far.

[JimBobSquarePants]

Man... I was really hoping that idea would get canned by the end of the conversation. Disappointing to see it blunt forced in despite valid opposition.

[antonfirsov]

Me and that debate:

[antonfirsov]

It's coming in #1296!