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

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

Open
benmccallum opened this issue Sep 18, 2020 · 3 comments

Comments

@benmccallum
Copy link

benmccallum commented Sep 18, 2020

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
Copy link
Author

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
Copy link
Author

benmccallum commented Mar 19, 2021

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
Copy link

@benmccallum nice approach 👍

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

No branches or pull requests

2 participants