OData / WebApi

OData Web API: A server library built upon ODataLib and WebApi
https://docs.microsoft.com/odata
Other
854 stars 473 forks source link

Returning an IQueryable from EF core 6 generates synchronous database calls #2598

Open jr01 opened 3 years ago

jr01 commented 3 years ago

On .Net 5 / MVC 5 + EF Core 5 when an controller action with [EnableQuery] returned an EF Core IQuerable which implemented IAsyncEnumerable the database call would be executed asynchronously.

With .Net 6 / MVC 6 + EF Core 6 the database call is executed synchronously.

Assemblies affected

Microsoft.AspNetCore.OData 8.0.4

Reproduce steps

In an ASP.Net 6 + oData 8.0.4 + EF core 6 project:

[EnableQuery]
public IActionResult Get()
{
    var query = this.dbContext.Persons.AsQueryable(); // implements IAsyncEnumerable
    return this.Ok(query);
}

...

public class BlockNonAsyncQueriesInterceptor : DbCommandInterceptor
    {
        private const string SyncCodeError = @"Synchronous code is not allowed for performance reasons.";

        public override InterceptionResult<int> NonQueryExecuting(DbCommand command, CommandEventData eventData, InterceptionResult<int> result)
        {
             throw new InvalidOperationException(SyncCodeError);
        }

        public override InterceptionResult<DbDataReader> ReaderExecuting(DbCommand command, CommandEventData eventData, InterceptionResult<DbDataReader> result)
        {
             throw new InvalidOperationException(SyncCodeError);
        }

        public override InterceptionResult<object> ScalarExecuting(DbCommand command, CommandEventData eventData, InterceptionResult<object> result)
        {
             throw new InvalidOperationException(SyncCodeError);
        }
    }
...

public class MyDbContext : DbContext
 protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
        optionsBuilder.AddInterceptors(new BlockNonAsyncQueriesInterceptor());

Expected result

Asynchronous DB calls are made.

Actual result

A synchronous DB call is made - not good for scaling/performance. With the BlockNonAsyncQueriesInterceptor the exception occurs at the foreach loop here: https://github.com/OData/WebApi/blob/master/src/Microsoft.AspNet.OData.Shared/Formatter/Serialization/ODataResourceSetSerializer.cs#L230

Additional detail

I'm not sure if this is related at all, but I found MVC 5 buffered IAsyncEnumerable and this got removed in MVC 6 - https://docs.microsoft.com/en-us/dotnet/core/compatibility/aspnet-core/6.0/iasyncenumerable-not-buffered-by-mvc

Anyway I found a workaround by introducing an EnableQueryAsync attribute and execute the IAsyncEnumerable and buffer the results in memory:

public sealed class EnableQueryAsyncAttribute : EnableQueryAttribute
    {
        private static readonly MethodInfo ReadInternalMethod = typeof(EnableQueryAsyncAttribute).GetMethods(BindingFlags.Static | BindingFlags.NonPublic)
           .Where(method => method.Name == nameof(EnableQueryAsyncAttribute.ReadInternal))
           .Single();

        public override async Task OnResultExecutionAsync(ResultExecutingContext context, ResultExecutionDelegate next)
        {
            if (context.Result is ObjectResult objectResult &&
                objectResult.Value is IQueryable queryable)
            {
                var cancellationToken = context.HttpContext.RequestAborted;

                var queryableType = queryable.GetType();
                if (queryableType.GetInterfaces().Any(x =>
                    x.IsGenericType &&
                    x.GetGenericTypeDefinition() == typeof(IAsyncEnumerable<>)))
                {
                    var readInternalMethod = ReadInternalMethod.MakeGenericMethod(queryableType.GenericTypeArguments[0]);

                    var invoked = readInternalMethod.Invoke(null, new object[] { queryable, cancellationToken })!;

                    var result = await (Task<ICollection>)invoked;

                    objectResult.Value = result;
                }
            }

            _ = await next().ConfigureAwait(false);
        }

        private static async Task<ICollection> ReadInternal<T>(object value, CancellationToken cancellationToken)
        {
            var asyncEnumerable = (IAsyncEnumerable<T>)value;
            var result = new List<T>();

            await foreach (var item in asyncEnumerable)
            {
                cancellationToken.ThrowIfCancellationRequested();

                result.Add(item);
            }

            return result;
        }
    }

Maybe a solution is if ODataResourceSetSerializer and other collection serializers check if IEnumerable is an IAsyncEnumerable and performs an await foreach ?

ElizabethOkerio commented 3 years ago

@jr01 we welcome a contribution on this. Feel free to do a PR and we'll be happy to review and merge. Thanks.

wbuck commented 3 years ago

@jr01 Are you planning on submitting a PR for this? I also just noticed this and would be interested in this feature being added.

jr01 commented 3 years ago

@wbuck @ElizabethOkerio The workaround is good enough for our current needs. Also we have been happy with a workaround for count async https://github.com/OData/WebApi/issues/2325 for about a year now ...

mbrankintrintech commented 2 years ago

+1 on this issue.

ElizabethOkerio commented 1 year ago

@jr01 we added support for IAsncyEnumerable using await foreach in the ODataResourceSetSerializer in the latest release of https://www.nuget.org/packages/Microsoft.AspNetCore.OData/. You could try it out and let us know if it works for you.

mbrankintrintech commented 1 year ago

@ElizabethOkerio Does this mean that in the latest update, returning IAsyncQueryable from the controller will make an async call to the database without any config changes?

If so, that's absolutely brilliant. kudos.

jr01 commented 1 year ago

@ElizabethOkerio with the steps above and the latest release (8.2.3) the code still throws in BlockNonAsyncQueriesInterceptor. Also I don't see any await foreach in https://github.com/OData/WebApi/blob/master/src/Microsoft.AspNet.OData.Shared/Formatter/Serialization/ODataResourceSetSerializer.cs ?

image

mbrankintrintech commented 1 year ago

Wouldn't it be here instead?

https://github.com/OData/AspNetCoreOData/blob/8.2.3/src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataResourceSetSerializer.cs

I do see differences there.

jr01 commented 1 year ago

Just decompiled 8.2.3 with dotpeek. Looks like that tag doesn't match with the published nuget.

image

ElizabethOkerio commented 1 year ago

This should be the right repo to look at: https://github.com/OData/AspNetCoreOData/blob/main/src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataResourceSetSerializer.cs

Having such a controller action method will make async calls.

         [HttpGet]
         [EnableQuery]
         public IAsyncEnumerable<Customer> Get()
         {
             return this.context.Customers.AsAsyncEnumerable();
         }
mbrankintrintech commented 1 year ago

@ElizabethOkerio Is 8.2.3 the release with this in it?

ElizabethOkerio commented 1 year ago

yes.

ElizabethOkerio commented 1 year ago

@mbrankintrintech @jr01 we'd like feedback on this on whether it works as expected and if there are any improvements that we can make.

mbrankintrintech commented 1 year ago

The big feedback for me would be that (if that this works) let's expand it to the other functions, namely the count command so that the count query is also non-blocking. 😄

ElizabethOkerio commented 1 year ago

Thanks. Yes, this is in our backlog.

jr01 commented 1 year ago

@ElizabethOkerio I think I was mistaken before and I now do see the IAsyncEnumerable code. Not sure what went wrong :)

Yes, the following is now making a non-blocking async call to the database, so that's great!:

[EnableQuery]
public IAsyncEnumerable<Person> Get()
{
     var query = this.dbContext.Persons.AsAsyncEnumerable();
     return query;
 }

However, when returning an ActionResult with an encapsulated IAsyncEnumerable it still runs synchronously:

 [EnableQuery]
 public ActionResult<IAsyncEnumerable<Person>> Get()
 {
     var query = this.dbContext.Persons.AsAsyncEnumerable();
     return this.Ok(query);
 }

image

Maybe this would work when this part of the check wasn't done?:

writeContext.Type != null &&
                writeContext.Type.IsGenericType &&
                writeContext.Type.GetGenericTypeDefinition() == typeof(IAsyncEnumerable<>)

Another thing that doesn't seem to work is an async IAsyncEnumerable<> - this returns 400 badrequest -- the Get method isn't discovered/executed at all:

 [EnableQuery]
 public async IAsyncEnumerable<Person> Get([EnumeratorCancellation] CancellationToken cancellationToken)
 {
    await Task.Delay(10, cancellationToken);

    var query = this.dbContext.Persons.AsAsyncEnumerable();

    await foreach (var item in query)
    {
        yield return item;
    }
 }
ElizabethOkerio commented 1 year ago

@jr01 Thanks. I'll look into this.

DimaMegaMan commented 1 year ago

@ElizabethOkerio I hope I'm not late, but according to the AsAsyncEnumerable implementation in EF Core:

public static IAsyncEnumerable<TSource> AsAsyncEnumerable<TSource>(
    this IQueryable<TSource> source)
{
    Check.NotNull(source, nameof(source));

    if (source is IAsyncEnumerable<TSource> asyncEnumerable)
    {
        return asyncEnumerable;
    }

    throw new InvalidOperationException(CoreStrings.IQueryableNotAsync(typeof(TSource)));
}

The object representing EF Core's IQueryable already implements IAsyncEnumerable, so it might be beneficial to add a simple check for the IAsyncEnumerable interface to make it compatible with all existing EF Core queries. With this check, we can eliminate the need for the AsAsyncEnumerable call and enable asynchronous request serialization where possible.

I believe this is a critical enhancement because query execution times can extend to tens of seconds or even minutes in practical scenarios. Prolonged synchronous queries can lead to thread starvation. Thus, implementing this straightforward check can significantly boost OData performance in real-world use cases.

ElizabethOkerio commented 1 year ago

@DimaMegaMan I could be wrong here but I don't think the source System.Linq.IQueryable implements IAsyncEnumerable. I would also like to understand whether you mean that having such


[EnableQuery]
public IQueryable<Person> Get()
{
    return this.dbContext.Persons;
}

should lead to async database calls being made? Currently, If you return an IQueryable that implements IAsyncEnumerable then async database calls are made..

Shiney commented 8 months ago

I think I have an explanation of what might be happening with this not working by default in the aspnet core version of this here https://github.com/OData/AspNetCoreOData/issues/1194

aldrashan commented 7 months ago

I'm using the [EnableQuery] attribute inside a normal [ApiController]. This works when I return a Task<IQueryable>, but fails with the following stacktrace when I return a Task<IAsyncEnumerable>:

System.ArgumentNullException: Value cannot be null. (Parameter 'entityClrType')
   at Microsoft.AspNetCore.OData.Extensions.ActionDescriptorExtensions.GetEdmModel(ActionDescriptor actionDescriptor, HttpRequest request, Type entityClrType)
   at Microsoft.AspNetCore.OData.Query.EnableQueryAttribute.GetModel(Type elementClrType, HttpRequest request, ActionDescriptor actionDescriptor)
   at Microsoft.AspNetCore.OData.Query.EnableQueryAttribute.CreateQueryOptionsOnExecuting(ActionExecutingContext actionExecutingContext)
   at Microsoft.AspNetCore.OData.Query.EnableQueryAttribute.OnActionExecuting(ActionExecutingContext actionExecutingContext)
   at Microsoft.AspNetCore.Mvc.Filters.ActionFilterAttribute.OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)

I'm not defining EdmModels or ClrTypes anywhere for the IQueryable code, so why would returning IAsyncEnumerable suddenly complain about it?

ElizabethOkerio commented 7 months ago

@aldrashan We'll look into this.

peter-bertok commented 7 months ago

I've been investigating the available options for improving the performance of OData for large data sets. Essentially, I hit a brick wall due to this issue.

From what I'm seeing, the root cause is the use of TruncatedCollection, which forces the query to be both synchronous and completely held in RAM. For large data volumes, this uses gigabytes of server memory, even with relatively small page sizes.

Conversely, the recently merged support for IAsyncEnumerable<> has great performance with a constant amount of very low memory usage. However, it is never used by EnableQuery, because it implicitly converts EF IQueryable data into this truncated collection instead of an enumerable.

A possible solution might be to replace TruncatedCollection with TruncatedEnumerable or something similar that allows async enumeration of large data sets.