Merge pull request #160 from deanmarcussen/dm/zerolength

JimBobSquarePants · web-flow · commit fa183578c358 · 2021-05-04T00:49:47.000+01:00
Fix Zero Length Images
diff --git a/src/ImageSharp.Web/Middleware/ConcurrentDictionaryExtensions.cs b/src/ImageSharp.Web/Middleware/ConcurrentDictionaryExtensions.cs
@@ -0,0 +1,63 @@
+// Copyright (c) Six Labors.
+// Licensed under the Apache License, Version 2.0.
+
+using System;
+using System.Collections.Concurrent;
+using System.Threading.Tasks;
+
+namespace SixLabors.ImageSharp.Web.Middleware
+{
+    /// <summary>
+    /// Extensions used to manage asynchronous access to the <see cref="ImageSharpMiddleware"/>
+    /// https://gist.github.com/davidfowl/3dac8f7b3d141ae87abf770d5781feed
+    /// </summary>
+    public static class ConcurrentDictionaryExtensions
+    {
+        /// <summary>
+        /// Provides an alternative to <see cref="ConcurrentDictionary{TKey, TValue}.GetOrAdd(TKey, Func{TKey, TValue})"/> specifically for asynchronous values. The factory method will only run once.
+        /// </summary>
+        /// <typeparam name="TKey">The type of the key.</typeparam>
+        /// <typeparam name="TValue">The value for the dictionary.</typeparam>
+        /// <param name="dictionary">The <see cref="ConcurrentDictionary{TKey, TValue}"/>.</param>
+        /// <param name="key">The key of the element to add.</param>
+        /// <param name="valueFactory">The function used to generate a value for the key</param>
+        /// <returns>The value for the key.  This will be either the existing value for the key if the
+        /// key is already in the dictionary, or the new value for the key as returned by valueFactory
+        /// if the key was not in the dictionary.</returns>
+        public static async Task<TValue> GetOrAddAsync<TKey, TValue>(
+            this ConcurrentDictionary<TKey, Task<TValue>> dictionary,
+            TKey key,
+            Func<TKey, Task<TValue>> valueFactory)
+        {
+            while (true)
+            {
+                if (dictionary.TryGetValue(key, out var task))
+                {
+                    return await task;
+                }
+
+                // This is the task that we'll return to all waiters. We'll complete it when the factory is complete
+                var tcs = new TaskCompletionSource<TValue>(TaskCreationOptions.RunContinuationsAsynchronously);
+                if (dictionary.TryAdd(key, tcs.Task))
+                {
+                    try
+                    {
+                        var value = await valueFactory(key);
+                        tcs.TrySetResult(value);
+                        return await tcs.Task;
+                    }
+                    catch (Exception ex)
+                    {
+                        // Make sure all waiters see the exception
+                        tcs.SetException(ex);
+
+                        // We remove the entry if the factory failed so it's not a permanent failure
+                        // and future gets can retry (this could be a pluggable policy)
+                        dictionary.TryRemove(key, out _);
+                        throw;
+                    }
+                }
+            }
+        }
+    }
+}
diff --git a/src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs b/src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs
@@ -32,14 +32,14 @@ public class ImageSharpMiddleware
         /// <summary>
         /// The write worker used for limiting identical requests.
         /// </summary>
-        private static readonly ConcurrentDictionary<string, Lazy<Task>> WriteWorkers
-            = new ConcurrentDictionary<string, Lazy<Task>>(StringComparer.OrdinalIgnoreCase);
+        private static readonly ConcurrentDictionary<string, Task<ImageWorkerResult>> WriteWorkers
+            = new ConcurrentDictionary<string, Task<ImageWorkerResult>>(StringComparer.OrdinalIgnoreCase);
 
         /// <summary>
         /// The read worker used for limiting identical requests.
         /// </summary>
-        private static readonly ConcurrentDictionary<string, Lazy<Task<ValueTuple<bool, ImageMetadata>>>> ReadWorkers
-            = new ConcurrentDictionary<string, Lazy<Task<ValueTuple<bool, ImageMetadata>>>>(StringComparer.OrdinalIgnoreCase);
+        private static readonly ConcurrentDictionary<string, Task<ImageWorkerResult>> ReadWorkers
+            = new ConcurrentDictionary<string, Task<ImageWorkerResult>>(StringComparer.OrdinalIgnoreCase);
 
         /// <summary>
         /// Used to temporarily store source metadata reads to reduce the overhead of cache lookups.
@@ -251,30 +251,40 @@ private async Task ProcessRequestAsync(
 
             // Check the cache, if present, not out of date and not requiring and update
             // we'll simply serve the file from there.
-            (bool newOrUpdated, ImageMetadata sourceImageMetadata) =
-                await this.IsNewOrUpdatedAsync(sourceImageResolver, imageContext, key);
+            ImageWorkerResult readResult = default;
+            try
+            {
+                readResult = await this.IsNewOrUpdatedAsync(sourceImageResolver, imageContext, key);
+            }
+            finally
+            {
+                ReadWorkers.TryRemove(key, out Task<ImageWorkerResult> _);
+            }
 
-            if (!newOrUpdated)
+            if (!readResult.IsNewOrUpdated)
             {
+                await this.SendResponseAsync(imageContext, key, readResult.CacheImageMetadata, readResult.Resolver);
                 return;
             }
 
-            // Not cached? Let's get it from the image resolver.
-            RecyclableMemoryStream outStream = null;
+            // Not cached, or is updated? Let's get it from the image resolver.
+            var sourceImageMetadata = readResult.SourceImageMetadata;
 
-            // Enter a write lock which locks writing and any reads for the same request.
-            // This reduces the overheads of unnecessary processing plus avoids file locks.
-            await WriteWorkers.GetOrAdd(
-                key,
-                _ => new Lazy<Task>(
-                    async () =>
+            // Enter an asynchronous write worker which prevents multiple writes and delays any reads for the same request.
+            // This reduces the overheads of unnecessary processing.
+            try
+            {
+                ImageWorkerResult writeResult = await WriteWorkers.GetOrAddAsync(
+                    key,
+                    async (key) =>
                     {
+                        RecyclableMemoryStream outStream = null;
                         try
                         {
                             // Prevent a second request from starting a read during write execution.
-                            if (ReadWorkers.TryGetValue(key, out Lazy<Task<(bool, ImageMetadata)>> readWork))
+                            if (ReadWorkers.TryGetValue(key, out Task<ImageWorkerResult> readWork))
                             {
-                                await readWork.Value;
+                                await readWork;
                             }
 
                             ImageCacheMetadata cachedImageMetadata = default;
@@ -334,11 +344,27 @@ await WriteWorkers.GetOrAdd(
                             // Save the image to the cache and send the response to the caller.
                             await this.cache.SetAsync(key, outStream, cachedImageMetadata);
 
-                            // Remove the resolver from the cache so we always resolve next request
+                            // Remove any resolver from the cache so we always resolve next request
                             // for the same key.
                             CacheResolverLru.TryRemove(key);
 
-                            await this.SendResponseAsync(imageContext, key, cachedImageMetadata, outStream, null);
+                            // Place the resolver in the lru cache.
+                            (IImageCacheResolver ImageCacheResolver, ImageCacheMetadata ImageCacheMetadata) cachedImage = await
+                                CacheResolverLru.GetOrAddAsync(
+                                    key,
+                                    async k =>
+                                    {
+                                        IImageCacheResolver resolver = await this.cache.GetAsync(k);
+                                        ImageCacheMetadata metadata = default;
+                                        if (resolver != null)
+                                        {
+                                            metadata = await resolver.GetMetaDataAsync();
+                                        }
+
+                                        return (resolver, metadata);
+                                    });
+
+                            return new ImageWorkerResult(cachedImage.ImageCacheMetadata, cachedImage.ImageCacheResolver);
                         }
                         catch (Exception ex)
                         {
@@ -350,9 +376,17 @@ await WriteWorkers.GetOrAdd(
                         finally
                         {
                             await this.StreamDisposeAsync(outStream);
-                            WriteWorkers.TryRemove(key, out Lazy<Task> _);
                         }
-                    }, LazyThreadSafetyMode.ExecutionAndPublication)).Value;
+                    });
+
+                await this.SendResponseAsync(imageContext, key, writeResult.CacheImageMetadata, writeResult.Resolver);
+            }
+            finally
+            {
+                // As soon as we have sent a response from a writer the result is available from a reader so we remove this task.
+                // Any existing awaiters will continue to await.
+                WriteWorkers.TryRemove(key, out Task<ImageWorkerResult> _);
+            }
         }
 
         private ValueTask StreamDisposeAsync(Stream stream)
@@ -377,85 +411,72 @@ private ValueTask StreamDisposeAsync(Stream stream)
 #endif
         }
 
-        private async Task<ValueTuple<bool, ImageMetadata>> IsNewOrUpdatedAsync(
+        private async Task<ImageWorkerResult> IsNewOrUpdatedAsync(
             IImageResolver sourceImageResolver,
             ImageContext imageContext,
             string key)
         {
-            if (WriteWorkers.TryGetValue(key, out Lazy<Task> writeWork))
+            // Pause until the write has been completed.
+            if (WriteWorkers.TryGetValue(key, out Task<ImageWorkerResult> writeWorkResult))
             {
-                await writeWork.Value;
+                return await writeWorkResult;
             }
 
-            if (ReadWorkers.TryGetValue(key, out Lazy<Task<(bool, ImageMetadata)>> readWork))
-            {
-                return await readWork.Value;
-            }
-
-            return await ReadWorkers.GetOrAdd(
+            return await ReadWorkers.GetOrAddAsync(
                 key,
-                _ => new Lazy<Task<ValueTuple<bool, ImageMetadata>>>(
-                    async () =>
+                async (key) =>
                     {
-                        try
-                        {
-                            // Get the source metadata for processing, storing the result for future checks.
-                            ImageMetadata sourceImageMetadata = await
-                                SourceMetadataLru.GetOrAddAsync(
-                                    key,
-                                    _ => sourceImageResolver.GetMetaDataAsync());
-
-                            // Check to see if the cache contains this image.
-                            // If not, we return early. No further checks necessary.
-                            (IImageCacheResolver ImageCacheResolver, ImageCacheMetadata ImageCacheMetadata) cachedImage = await
-                                CacheResolverLru.GetOrAddAsync(
-                                    key,
-                                    async k =>
+                        // Get the source metadata for processing, storing the result for future checks.
+                        ImageMetadata sourceImageMetadata = await
+                            SourceMetadataLru.GetOrAddAsync(
+                                key,
+                                _ => sourceImageResolver.GetMetaDataAsync());
+
+                        // Check to see if the cache contains this image.
+                        // If not, we return early. No further checks necessary.
+                        (IImageCacheResolver ImageCacheResolver, ImageCacheMetadata ImageCacheMetadata) cachedImage = await
+                            CacheResolverLru.GetOrAddAsync(
+                                key,
+                                async k =>
+                                {
+                                    IImageCacheResolver resolver = await this.cache.GetAsync(k);
+                                    ImageCacheMetadata metadata = default;
+                                    if (resolver != null)
                                     {
-                                        IImageCacheResolver resolver = await this.cache.GetAsync(k);
-                                        ImageCacheMetadata metadata = default;
-                                        if (resolver != null)
-                                        {
-                                            metadata = await resolver.GetMetaDataAsync();
-                                        }
-
-                                        return (resolver, metadata);
-                                    });
+                                        metadata = await resolver.GetMetaDataAsync();
+                                    }
 
-                            if (cachedImage.ImageCacheResolver is null)
-                            {
-                                // Remove the null resolver from the store.
-                                CacheResolverLru.TryRemove(key);
-                                return (true, sourceImageMetadata);
-                            }
+                                    return (resolver, metadata);
+                                });
 
-                            // Has the cached image expired?
-                            // Or has the source image changed since the image was last cached?
-                            if (cachedImage.ImageCacheMetadata.ContentLength == 0 // Fix for old cache without length property
-                                || cachedImage.ImageCacheMetadata.CacheLastWriteTimeUtc <= (DateTimeOffset.UtcNow - this.options.CacheMaxAge)
-                                || cachedImage.ImageCacheMetadata.SourceLastWriteTimeUtc != sourceImageMetadata.LastWriteTimeUtc)
-                            {
-                                // We want to remove the resolver from the store so that the next check gets the updated file.
-                                CacheResolverLru.TryRemove(key);
-                                return (true, sourceImageMetadata);
-                            }
+                        if (cachedImage.ImageCacheResolver is null)
+                        {
+                            // Remove the null resolver from the store.
+                            CacheResolverLru.TryRemove(key);
 
-                            // We're pulling the image from the cache.
-                            await this.SendResponseAsync(imageContext, key, cachedImage.ImageCacheMetadata, null, cachedImage.ImageCacheResolver);
-                            return (false, sourceImageMetadata);
+                            return new ImageWorkerResult(sourceImageMetadata);
                         }
-                        finally
+
+                        // Has the cached image expired?
+                        // Or has the source image changed since the image was last cached?
+                        if (cachedImage.ImageCacheMetadata.ContentLength == 0 // Fix for old cache without length property
+                            || cachedImage.ImageCacheMetadata.CacheLastWriteTimeUtc <= (DateTimeOffset.UtcNow - this.options.CacheMaxAge)
+                            || cachedImage.ImageCacheMetadata.SourceLastWriteTimeUtc != sourceImageMetadata.LastWriteTimeUtc)
                         {
-                            ReadWorkers.TryRemove(key, out Lazy<Task<(bool, ImageMetadata)>> _);
+                            // We want to remove the resolver from the store so that the next check gets the updated file.
+                            CacheResolverLru.TryRemove(key);
+                            return new ImageWorkerResult(sourceImageMetadata);
                         }
-                    }, LazyThreadSafetyMode.ExecutionAndPublication)).Value;
+
+                        // The image is cached. Return the cached image so multiple callers can write a response.
+                        return new ImageWorkerResult(sourceImageMetadata, cachedImage.ImageCacheMetadata, cachedImage.ImageCacheResolver);
+                    });
         }
 
         private async Task SendResponseAsync(
             ImageContext imageContext,
             string key,
             ImageCacheMetadata metadata,
-            Stream stream,
             IImageCacheResolver cacheResolver)
         {
             imageContext.ComprehendRequestHeaders(metadata.CacheLastWriteTimeUtc, metadata.ContentLength);
@@ -473,7 +494,11 @@ private async Task SendResponseAsync(
                     this.logger.LogImageServed(imageContext.GetDisplayUrl(), key);
 
                     // When stream is null we're sending from the cache.
-                    await imageContext.SendAsync(stream ?? await cacheResolver.OpenReadAsync(), metadata);
+                    using (var stream = await cacheResolver.OpenReadAsync())
+                    {
+                        await imageContext.SendAsync(stream, metadata);
+                    }
+
                     return;
 
                 case ImageContext.PreconditionState.NotModified:
diff --git a/src/ImageSharp.Web/Middleware/ImageWorkerResult.cs b/src/ImageSharp.Web/Middleware/ImageWorkerResult.cs
@@ -0,0 +1,45 @@
+// Copyright (c) Six Labors.
+// Licensed under the Apache License, Version 2.0.
+
+using SixLabors.ImageSharp.Web.Resolvers;
+
+namespace SixLabors.ImageSharp.Web.Middleware
+{
+    /// <summary>
+    /// Provides an asynchronous worker result.
+    /// </summary>
+    internal readonly struct ImageWorkerResult
+    {
+        public ImageWorkerResult(ImageMetadata sourceImageMetadata)
+        {
+            this.IsNewOrUpdated = true;
+            this.SourceImageMetadata = sourceImageMetadata;
+            this.CacheImageMetadata = default;
+            this.Resolver = default;
+        }
+
+        public ImageWorkerResult(ImageMetadata sourceImageMetadata, ImageCacheMetadata cacheImageMetadata, IImageCacheResolver resolver)
+        {
+            this.IsNewOrUpdated = false;
+            this.SourceImageMetadata = sourceImageMetadata;
+            this.CacheImageMetadata = cacheImageMetadata;
+            this.Resolver = resolver;
+        }
+
+        public ImageWorkerResult(ImageCacheMetadata cacheImageMetadata, IImageCacheResolver resolver)
+        {
+            this.IsNewOrUpdated = false;
+            this.SourceImageMetadata = default;
+            this.CacheImageMetadata = cacheImageMetadata;
+            this.Resolver = resolver;
+        }
+
+        public bool IsNewOrUpdated { get; }
+
+        public ImageMetadata SourceImageMetadata { get; }
+
+        public ImageCacheMetadata CacheImageMetadata { get; }
+
+        public IImageCacheResolver Resolver { get; }
+    }
+}
diff --git a/tests/ImageSharp.Web.Tests/Processing/AzureBlobStorageCacheServerTests.cs b/tests/ImageSharp.Web.Tests/Processing/AzureBlobStorageCacheServerTests.cs
@@ -111,6 +111,7 @@ public async Task CanProcessMultipleIdenticalQueriesAsync(string url)
                 using HttpResponseMessage response = await this.HttpClient.GetAsync(url + command);
                 Assert.NotNull(response);
                 Assert.True(response.IsSuccessStatusCode);
+                Assert.True(response.Content.Headers.ContentLength > 0);
             })).ToArray();
 
             var all = Task.WhenAll(tasks);
diff --git a/tests/ImageSharp.Web.Tests/Processing/PhysicalFileSystemCacheServerTests.cs b/tests/ImageSharp.Web.Tests/Processing/PhysicalFileSystemCacheServerTests.cs
@@ -111,6 +111,7 @@ public async Task CanProcessMultipleIdenticalQueriesAsync(string url)
                   using HttpResponseMessage response = await this.HttpClient.GetAsync(url + command);
                   Assert.NotNull(response);
                   Assert.True(response.IsSuccessStatusCode);
+                  Assert.True(response.Content.Headers.ContentLength > 0);
               })).ToArray();
 
             var all = Task.WhenAll(tasks);
