Dasync / AsyncEnumerable

Defines IAsyncEnumerable, IAsyncEnumerator, ForEachAsync(), ParallelForEachAsync(), and other useful stuff to use with async-await
MIT License
444 stars 56 forks source link

Accidentally using Dasync's async methods on an EF DbSet blows up the world #64

Open benmccallum opened 4 years ago

benmccallum commented 4 years ago
using Dasync.Collections;

var booking = _dbContext.Bookings.SingleAsync(b => b.Id == id, cancellationToken);

This will result in the booking sometimes being grabbed, but in most cases it seems like much more happens than you want, e.g. it crawls all the relations, consumes a tonne of memory and eventually crashes your app.

I'm not sure how we can really solve this other than an analyzer that detects usage on a DbSet<T> and says, "No, don't do this".

cc: @anthony-keller

benmccallum commented 3 years ago

This has happened to us again but was caught before deployment to prod just in time. It's unfortunately way to easy to accidentally include the DAsync.Collections using rather than the EF Core one and end up in a really bad place, i.e. your app using memory until it crashes.

We're considering just removing DAsync as we only use it for ParallelForEach with a limiter and will search for an equivalent.

I know it's not the fault of this library; I'm just not sure what to do...

benmccallum commented 3 years ago

We've since written a unit test that scans source files for uses of the Dasync namespace and forces us to manually verify new uses.

Posting here for anyone who finds it useful. Credit to Anth for the idea and impl.

cc: @simoncropp, another fun use of Verify ;)

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Threading.Tasks;
using VerifyXunit;
using Xunit;

namespace MyCompany.MyTests
{
    /// <summary>
    /// Reads all C# files and checks for lines starting with 'using Dasync'. The aim
    /// of this test is to highlight new usages of this library to ensure it's not
    /// being inadvertently used instead of identically-named Entity Framework methods.
    ///
    /// e.g. .FirstOrDefaultAsync()
    /// </summary>
    [UsesVerify]
    public class DasyncUsageTests
    {
        private const string _dAsyncUsing = "using Dasync";

        [Fact]
        public async Task Should_Only_Use_Dasync_When_Verified()
        {
            var assemblyPath = System.Reflection.Assembly.GetExecutingAssembly().Location;
            var assemblyPathParts = assemblyPath.Split(Path.DirectorySeparatorChar);
            var microservicePath = Path.Combine(assemblyPathParts.TakeWhile(s => s != "Microservices").ToArray());

            var csharpFiles = Directory.GetFiles(microservicePath, "*.cs", SearchOption.AllDirectories);
            var indexToTakeFrom = new Lazy<int>(() =>
                csharpFiles.First().IndexOf($"{Path.DirectorySeparatorChar}src{Path.DirectorySeparatorChar}"));

            var filesUsingDasync = new List<string>();

            foreach (var csharpFile in csharpFiles)
            {
                var lines = await File.ReadAllLinesAsync(csharpFile);
                if (lines.Any(s => s.StartsWith(_dAsyncUsing)))
                {
                    var universalFilePath = (indexToTakeFrom.Value < 1
                        ? csharpFile
                        : csharpFile.Substring(indexToTakeFrom.Value))
                        .Replace(@"\", @"/");

                    filesUsingDasync.Add(universalFilePath);
                }
            }

            await Verifier.Verify(filesUsingDasync);
        }
    }
}
SimonCropp commented 3 years ago

@benmccallum nice approach 👍