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

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

Open
jr01 opened this issue Nov 12, 2021 · 23 comments
Open

Comments

@jr01
Copy link

jr01 commented Nov 12, 2021

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

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

@wbuck
Copy link

wbuck commented Nov 21, 2021

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

jr01 commented Nov 25, 2021

@wbuck @ElizabethOkerio The workaround is good enough for our current needs. Also we have been happy with a workaround for count async #2325 for about a year now ...

@mbrankintrintech
Copy link

+1 on this issue.

@ElizabethOkerio
Copy link
Contributor

ElizabethOkerio commented Oct 3, 2023

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

mbrankintrintech commented Oct 3, 2023

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

jr01 commented Oct 3, 2023

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

@jr01
Copy link
Author

jr01 commented Oct 3, 2023

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

image

@ElizabethOkerio
Copy link
Contributor

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

@ElizabethOkerio Is 8.2.3 the release with this in it?

@ElizabethOkerio
Copy link
Contributor

yes.

@ElizabethOkerio
Copy link
Contributor

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

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

Thanks. Yes, this is in our backlog.

@jr01
Copy link
Author

jr01 commented Oct 5, 2023

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

@jr01 Thanks. I'll look into this.

@DimaMegaMan
Copy link

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

ElizabethOkerio commented Oct 18, 2023

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

Shiney commented Mar 15, 2024

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 OData/AspNetCoreOData#1194

@aldrashan
Copy link

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

@aldrashan We'll look into this.

@peter-bertok
Copy link

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.

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

No branches or pull requests

8 participants