Purpose of Task.Run in GetQueryAsync (ASP.NET Core)

[stevendarby]

Hi

I’m interested by this code and trying to understand it. It’s not immediately obvious to me why this method is async, as the only async aspect is when some otherwise synchronous code is placed onto another thread with Task.Run.

https://github.com/AutoMapper/AutoMapper.Extensions.OData/blob/e5c3e496309c27ced1055b5405bca74488d44e0e/AutoMapper.AspNetCore.OData.EFCore/QueryableExtensions.cs#L123

It seems to be a generally agreed principle that Task.Run doesn’t make sense on ASP.NET Core because there is no main UI thread which you are trying to keep responsive by running long running code on another thread. Instead there are just many threads all serving different requests, and making a request use another thread with Task.Run reduces the number of available threads to serve requests and thus reduces scalability. It’s with this in mind that I’m interested in your use of it and hoping you could explain. Thanks

[BlaiseD]

Looks like you're correct given the recommendations here.

We should provide an additional non-async GetQuery method so we don't break existing callers. Ok to create a PR.

[BlaiseD]

Or maybe not a GetQuery method. GetQueryAsync (with the ODataQueryOptions<TModel> options argument) could use LongCountAsync. We'll just get rid of the async stuff for projection.

public static long LongCount<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate);
Task<long> LongCountAsync<TSource>(this IQueryable<TSource> source, Expression<Func<TSource, bool>> predicate, CancellationToken cancellationToken = default);

Thanks.

[stevendarby]

I’m particularly interested in the GetQueryAsync linked in original post- cannot see a long count there unless I’m missing something. So I propose adding a GetQuery version for it, and keeping GetQueryAsync but obsoleting it. GetQueryAsync will just call GetQuery in a Task.Run.

I would love to completely rework this class to properly use async in many of its other methods too, however a) I only use this one method so it is of particular interest to me b) I would like my first contribution to be small and focussed c) it may lead to a handful more obsolete methods (eg adding synchronous methods where async wasn’t needed, but maintaining the async ones for compatibility). As there are already a handful of obsolete ones, it becomes a bit unwieldy. May be something to revisit later.

Does that sound ok? Is it better to leave issue open until in place?

[BlaiseD]

Issues are really for bugs and other difficulties using the libraries. Ok to create a PR and go from there.

This GetQueryAsync could really be internal - it's unrelated OData. May want to consider just creating your own helper if that's all you're using.

        public static async Task<IQueryable<TModel>> GetQueryAsync<TModel, TData>(this IQueryable<TData> query, IMapper mapper,
            Expression<Func<TModel, bool>> filter = null,
            Expression<Func<IQueryable<TModel>, IQueryable<TModel>>> queryFunc = null,
            IEnumerable<Expression<Func<TModel, object>>> includeProperties = null,
            ProjectionSettings projectionSettings = null)
        {
//removed for brevity
        }

[BlaiseD]

Excessive Task.Runs should be resolved with PR #91 (v2.1.1-preview.0).