Skip to content
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

Replace IImageProvider.Get() with GetAsync() #48

Merged
merged 1 commit into from
Dec 29, 2018

Conversation

kroymann
Copy link
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This makes it possible for implementations of IImageProvider that rely on remote image sources to asynchronously create an IImageResolver. This is important as it allows them to determine whether the requested image exists or not in an asynchronous manner prior to returning the IImageResolver.

This makes it possible for implementations of IImageProvider that rely on remote image sources to asynchronously create an IImageResolver.  This is important as it allows them to determine whether the requested image exists or not in an asynchronous manner prior to returning the IImageResolver.
@CLAassistant
Copy link

CLAassistant commented Dec 29, 2018

CLA assistant check
All committers have signed the CLA.

@sebastienros
Copy link
Contributor

Interesting, I think this was changed in the last beta, and the async logic is actually in the IImageResolver implementation, such that this method should not need async, or at least that's how I understood it when I adapted the code.

@kroymann
Copy link
Contributor Author

The issue with having the async logic in the IImageResolver and not in the IImageProvider is that you have to determine whether or not the source image exists in the provider prior to creating the resolver. If the source image doesn't exist, then the IImageProvider.Get() method needs to return null. If instead you speculatively return an IImageResolver that later determines that the source image doesn't exist, there's no way to deal with it at that point. Attempting to return null from the IImageResolver.OpenReadAsync() method results in a NullReferenceException. I had a quick discussion about this with @JimBobSquarePants on gitter and he agreed that this change seemed like the simplest and most logical solution to the problem.

@sebastienros
Copy link
Contributor

I am not disagreeing. and If the boss said so then it's all good.

@JimBobSquarePants
Copy link
Member

Yeah, oversight on my part. No way to check if the file exists asynchronously in Azure for example. I'm gonna merge this in, and then build a separate project with an Azure blob resolver to make sure it's all sensible before shipping another beta.

@JimBobSquarePants JimBobSquarePants merged commit 03b3771 into SixLabors:master Dec 29, 2018
@kroymann
Copy link
Contributor Author

In case it's useful, here's a quick sample I threw together that uses an Azure Blob Storage account as the image source: https://gist.github.com/kroymann/8955c5ebb1d89a04c206f90d92a286c1

@kroymann kroymann deleted the async_image_provider branch December 29, 2018 05:29
@JimBobSquarePants
Copy link
Member

Very useful, thanks!

I've started putting together an official version based on your code. There's a few performance fixes in there plus access control settings

https://github.com/SixLabors/ImageSharp.Web/tree/2d784e65f445ff493b28870de8c1097ee648e8d0/src/ImageSharp.Web.Providers.Azure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants