Skip to content

Eagerly read IAsyncEnumerable{object} instances before formatting #11118

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

Merged
merged 5 commits into from
Jun 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -875,6 +875,7 @@ public MvcOptions() { }
public Microsoft.AspNetCore.Mvc.Filters.FilterCollection Filters { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public Microsoft.AspNetCore.Mvc.Formatters.FormatterMappings FormatterMappings { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public Microsoft.AspNetCore.Mvc.Formatters.FormatterCollection<Microsoft.AspNetCore.Mvc.Formatters.IInputFormatter> InputFormatters { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public int MaxIAsyncEnumerableBufferLimit { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public int MaxModelBindingCollectionSize { get { throw null; } set { } }
public int MaxModelBindingRecursionDepth { get { throw null; } set { } }
public int MaxModelValidationErrors { get { throw null; } set { } }
Expand Down Expand Up @@ -2085,7 +2086,9 @@ public MvcCompatibilityOptions() { }
}
public partial class ObjectResultExecutor : Microsoft.AspNetCore.Mvc.Infrastructure.IActionResultExecutor<Microsoft.AspNetCore.Mvc.ObjectResult>
{
[System.ObsoleteAttribute("This constructor is obsolete and will be removed in a future release.")]
public ObjectResultExecutor(Microsoft.AspNetCore.Mvc.Infrastructure.OutputFormatterSelector formatterSelector, Microsoft.AspNetCore.Mvc.Infrastructure.IHttpResponseStreamWriterFactory writerFactory, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory) { }
public ObjectResultExecutor(Microsoft.AspNetCore.Mvc.Infrastructure.OutputFormatterSelector formatterSelector, Microsoft.AspNetCore.Mvc.Infrastructure.IHttpResponseStreamWriterFactory writerFactory, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Mvc.MvcOptions> mvcOptions) { }
protected Microsoft.AspNetCore.Mvc.Infrastructure.OutputFormatterSelector FormatterSelector { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
protected Microsoft.Extensions.Logging.ILogger Logger { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
protected System.Func<System.IO.Stream, System.Text.Encoding, System.IO.TextWriter> WriterFactory { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
Expand Down
102 changes: 102 additions & 0 deletions src/Mvc/Mvc.Core/src/Infrastructure/AsyncEnumerableReader.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Reflection;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Mvc.Core;
using Microsoft.Extensions.Internal;

#if JSONNET
namespace Microsoft.AspNetCore.Mvc.NewtonsoftJson
#else
namespace Microsoft.AspNetCore.Mvc.Infrastructure
#endif
{
using ReaderFunc = Func<IAsyncEnumerable<object>, Task<ICollection>>;

/// <summary>
/// Type that reads an <see cref="IAsyncEnumerable{T}"/> instance into a
/// generic collection instance.
/// </summary>
/// <remarks>
/// This type is used to create a strongly typed synchronous <see cref="ICollection{T}"/> instance from
/// an <see cref="IAsyncEnumerable{T}"/>. An accurate <see cref="ICollection{T}"/> is required for XML formatters to
/// correctly serialize.
/// </remarks>
internal sealed class AsyncEnumerableReader
{
private readonly MethodInfo Converter = typeof(AsyncEnumerableReader).GetMethod(
nameof(ReadInternal),
BindingFlags.NonPublic | BindingFlags.Instance);

private readonly ConcurrentDictionary<Type, ReaderFunc> _asyncEnumerableConverters =
new ConcurrentDictionary<Type, ReaderFunc>();
private readonly MvcOptions _mvcOptions;

/// <summary>
/// Initializes a new instance of <see cref="AsyncEnumerableReader"/>.
/// </summary>
/// <param name="mvcOptions">Accessor to <see cref="MvcOptions"/>.</param>
public AsyncEnumerableReader(MvcOptions mvcOptions)
{
_mvcOptions = mvcOptions;
}

/// <summary>
/// Reads a <see cref="IAsyncEnumerable{T}"/> into an <see cref="ICollection{T}"/>.
/// </summary>
/// <param name="value">The <see cref="IAsyncEnumerable{T}"/> to read.</param>
/// <returns>The <see cref="ICollection"/>.</returns>
public Task<ICollection> ReadAsync(IAsyncEnumerable<object> value)
{
if (value == null)
{
throw new ArgumentNullException(nameof(value));
}

var type = value.GetType();
if (!_asyncEnumerableConverters.TryGetValue(type, out var result))
{
var enumerableType = ClosedGenericMatcher.ExtractGenericInterface(type, typeof(IAsyncEnumerable<>));
Debug.Assert(enumerableType != null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a public API, so this should probably throw instead of assert. Someone could pass in any type here.


var enumeratedObjectType = enumerableType.GetGenericArguments()[0];

var converter = (ReaderFunc)Converter
.MakeGenericMethod(enumeratedObjectType)
.CreateDelegate(typeof(ReaderFunc), this);

_asyncEnumerableConverters.TryAdd(type, converter);
result = converter;
}

return result(value);
}

private async Task<ICollection> ReadInternal<T>(IAsyncEnumerable<object> value)
{
var asyncEnumerable = (IAsyncEnumerable<T>)value;
var result = new List<T>();
var count = 0;

await foreach (var item in asyncEnumerable)
{
if (count++ >= _mvcOptions.MaxIAsyncEnumerableBufferLimit)
{
throw new InvalidOperationException(Resources.FormatObjectResultExecutor_MaxEnumerationExceeded(
nameof(AsyncEnumerableReader),
value.GetType()));
}

result.Add(item);
}

return result;
}
}
}
61 changes: 60 additions & 1 deletion src/Mvc/Mvc.Core/src/Infrastructure/ObjectResultExecutor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Formatters;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.Mvc.Infrastructure
{
Expand All @@ -18,16 +19,35 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure
/// </summary>
public class ObjectResultExecutor : IActionResultExecutor<ObjectResult>
{
private readonly AsyncEnumerableReader _asyncEnumerableReader;

/// <summary>
/// Creates a new <see cref="ObjectResultExecutor"/>.
/// </summary>
/// <param name="formatterSelector">The <see cref="OutputFormatterSelector"/>.</param>
/// <param name="writerFactory">The <see cref="IHttpResponseStreamWriterFactory"/>.</param>
/// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>
[Obsolete("This constructor is obsolete and will be removed in a future release.")]
public ObjectResultExecutor(
OutputFormatterSelector formatterSelector,
IHttpResponseStreamWriterFactory writerFactory,
ILoggerFactory loggerFactory)
: this(formatterSelector, writerFactory, loggerFactory, mvcOptions: null)
{
}

/// <summary>
/// Creates a new <see cref="ObjectResultExecutor"/>.
/// </summary>
/// <param name="formatterSelector">The <see cref="OutputFormatterSelector"/>.</param>
/// <param name="writerFactory">The <see cref="IHttpResponseStreamWriterFactory"/>.</param>
/// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>
/// <param name="mvcOptions">Accessor to <see cref="MvcOptions"/>.</param>
public ObjectResultExecutor(
OutputFormatterSelector formatterSelector,
IHttpResponseStreamWriterFactory writerFactory,
ILoggerFactory loggerFactory,
IOptions<MvcOptions> mvcOptions)
{
if (formatterSelector == null)
{
Expand All @@ -47,6 +67,8 @@ public ObjectResultExecutor(
FormatterSelector = formatterSelector;
WriterFactory = writerFactory.CreateWriter;
Logger = loggerFactory.CreateLogger<ObjectResultExecutor>();
var options = mvcOptions?.Value ?? throw new ArgumentNullException(nameof(mvcOptions));
_asyncEnumerableReader = new AsyncEnumerableReader(options);
}

/// <summary>
Expand Down Expand Up @@ -87,16 +109,37 @@ public virtual Task ExecuteAsync(ActionContext context, ObjectResult result)
InferContentTypes(context, result);

var objectType = result.DeclaredType;

if (objectType == null || objectType == typeof(object))
{
objectType = result.Value?.GetType();
}

var value = result.Value;

if (value is IAsyncEnumerable<object> asyncEnumerable)
{
return ExecuteAsyncEnumerable(context, result, asyncEnumerable);
}

return ExecuteAsyncCore(context, result, objectType, value);
}

private async Task ExecuteAsyncEnumerable(ActionContext context, ObjectResult result, IAsyncEnumerable<object> asyncEnumerable)
{
Log.BufferingAsyncEnumerable(Logger, asyncEnumerable);

var enumerated = await _asyncEnumerableReader.ReadAsync(asyncEnumerable);
await ExecuteAsyncCore(context, result, enumerated.GetType(), enumerated);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be logging here?

}

private Task ExecuteAsyncCore(ActionContext context, ObjectResult result, Type objectType, object value)
{
var formatterContext = new OutputFormatterWriteContext(
context.HttpContext,
WriterFactory,
objectType,
result.Value);
value);

var selectedFormatter = FormatterSelector.SelectFormatter(
formatterContext,
Expand Down Expand Up @@ -138,5 +181,21 @@ private static void InferContentTypes(ActionContext context, ObjectResult result
result.ContentTypes.Add("application/problem+xml");
}
}

private static class Log
{
private static readonly Action<ILogger, string, Exception> _bufferingAsyncEnumerable;

static Log()
{
_bufferingAsyncEnumerable = LoggerMessage.Define<string>(
LogLevel.Debug,
new EventId(1, "BufferingAsyncEnumerable"),
"Buffering IAsyncEnumerable instance of type '{Type}'.");
}

public static void BufferingAsyncEnumerable(ILogger logger, IAsyncEnumerable<object> asyncEnumerable)
=> _bufferingAsyncEnumerable(logger, asyncEnumerable.GetType().FullName, null);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.IO;
using System.Text;
using System.Text.Json;
Expand All @@ -25,13 +26,16 @@ internal sealed class SystemTextJsonResultExecutor : IActionResultExecutor<JsonR

private readonly JsonOptions _options;
private readonly ILogger<SystemTextJsonResultExecutor> _logger;
private readonly AsyncEnumerableReader _asyncEnumerableReader;

public SystemTextJsonResultExecutor(
IOptions<JsonOptions> options,
ILogger<SystemTextJsonResultExecutor> logger)
ILogger<SystemTextJsonResultExecutor> logger,
IOptions<MvcOptions> mvcOptions)
{
_options = options.Value;
_logger = logger;
_asyncEnumerableReader = new AsyncEnumerableReader(mvcOptions.Value);
}

public async Task ExecuteAsync(ActionContext context, JsonResult result)
Expand Down Expand Up @@ -70,8 +74,15 @@ public async Task ExecuteAsync(ActionContext context, JsonResult result)
var writeStream = GetWriteStream(context.HttpContext, resolvedContentTypeEncoding);
try
{
var type = result.Value?.GetType() ?? typeof(object);
await JsonSerializer.WriteAsync(writeStream, result.Value, type, jsonSerializerOptions);
var value = result.Value;
if (value is IAsyncEnumerable<object> asyncEnumerable)
{
Log.BufferingAsyncEnumerable(_logger, asyncEnumerable);
value = await _asyncEnumerableReader.ReadAsync(asyncEnumerable);
}

var type = value?.GetType() ?? typeof(object);
await JsonSerializer.WriteAsync(writeStream, value, type, jsonSerializerOptions);
await writeStream.FlushAsync();
}
finally
Expand Down Expand Up @@ -123,11 +134,19 @@ private static class Log
new EventId(1, "JsonResultExecuting"),
"Executing JsonResult, writing value of type '{Type}'.");

private static readonly Action<ILogger, string, Exception> _bufferingAsyncEnumerable = LoggerMessage.Define<string>(
LogLevel.Debug,
new EventId(2, "BufferingAsyncEnumerable"),
"Buffering IAsyncEnumerable instance of type '{Type}'.");

public static void JsonResultExecuting(ILogger logger, object value)
{
var type = value == null ? "null" : value.GetType().FullName;
_jsonResultExecuting(logger, type, null);
}

public static void BufferingAsyncEnumerable(ILogger logger, IAsyncEnumerable<object> asyncEnumerable)
=> _bufferingAsyncEnumerable(logger, asyncEnumerable.GetType().FullName, null);
}
}
}
13 changes: 13 additions & 0 deletions src/Mvc/Mvc.Core/src/MvcOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,19 @@ public int MaxModelBindingRecursionDepth
}
}

/// <summary>
/// Gets or sets the most number of entries of an <see cref="IAsyncEnumerable{T}"/> that
/// that <see cref="ObjectResultExecutor"/> will buffer.
/// <para>
/// When <see cref="ObjectResult.Value" /> is an instance of <see cref="IAsyncEnumerable{T}"/>,
/// <see cref="ObjectResultExecutor"/> will eagerly read the enumeration and add to a synchronous collection
/// prior to invoking the selected formatter.
/// This property determines the most number of entries that the executor is allowed to buffer.
/// </para>
/// </summary>
/// <value>Defaults to <c>8192</c>.</value>
public int MaxIAsyncEnumerableBufferLimit { get; set; } = 8192;

IEnumerator<ICompatibilitySwitch> IEnumerable<ICompatibilitySwitch>.GetEnumerator() => _switches.GetEnumerator();

IEnumerator IEnumerable.GetEnumerator() => _switches.GetEnumerator();
Expand Down
14 changes: 14 additions & 0 deletions src/Mvc/Mvc.Core/src/Properties/Resources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion src/Mvc/Mvc.Core/src/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -503,5 +503,8 @@
</data>
<data name="Property_MustBeInstanceOfType" xml:space="preserve">
<value>Property '{0}.{1}' must be an instance of type '{2}'.</value>
</data>
</data>
<data name="ObjectResultExecutor_MaxEnumerationExceeded" xml:space="preserve">
<value>'{0}' reached the configured maximum size of the buffer when enumerating a value of type '{1}'. This limit is in place to prevent infinite streams of 'IAsyncEnumerable&lt;&gt;' from continuing indefinitely. If this is not a programming mistake, consider ways to reduce the collection size, or consider manually converting '{1}' into a list rather than increasing the limit.</value>
</data>
</root>
3 changes: 2 additions & 1 deletion src/Mvc/Mvc.Core/test/AcceptedAtActionResultTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,8 @@ private static IServiceProvider CreateServices(Mock<IOutputFormatter> formatter)
services.AddSingleton<IActionResultExecutor<ObjectResult>>(new ObjectResultExecutor(
new DefaultOutputFormatterSelector(options, NullLoggerFactory.Instance),
new TestHttpResponseStreamWriterFactory(),
NullLoggerFactory.Instance));
NullLoggerFactory.Instance,
options));

return services.BuildServiceProvider();
}
Expand Down
3 changes: 2 additions & 1 deletion src/Mvc/Mvc.Core/test/AcceptedAtRouteResultTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ private static IServiceProvider CreateServices(Mock<IOutputFormatter> formatter)
services.AddSingleton<IActionResultExecutor<ObjectResult>>(new ObjectResultExecutor(
new DefaultOutputFormatterSelector(options, NullLoggerFactory.Instance),
new TestHttpResponseStreamWriterFactory(),
NullLoggerFactory.Instance));
NullLoggerFactory.Instance,
options));

return services.BuildServiceProvider();
}
Expand Down
3 changes: 2 additions & 1 deletion src/Mvc/Mvc.Core/test/AcceptedResultTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@ private static IServiceProvider CreateServices(Mock<IOutputFormatter> formatter)
services.AddSingleton<IActionResultExecutor<ObjectResult>>(new ObjectResultExecutor(
new DefaultOutputFormatterSelector(options, NullLoggerFactory.Instance),
new TestHttpResponseStreamWriterFactory(),
NullLoggerFactory.Instance));
NullLoggerFactory.Instance,
options));

return services.BuildServiceProvider();
}
Expand Down
3 changes: 2 additions & 1 deletion src/Mvc/Mvc.Core/test/CreatedAtActionResultTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ private static IServiceProvider CreateServices()
services.AddSingleton<IActionResultExecutor<ObjectResult>>(new ObjectResultExecutor(
new DefaultOutputFormatterSelector(options, NullLoggerFactory.Instance),
new TestHttpResponseStreamWriterFactory(),
NullLoggerFactory.Instance));
NullLoggerFactory.Instance,
options));

return services.BuildServiceProvider();
}
Expand Down
3 changes: 2 additions & 1 deletion src/Mvc/Mvc.Core/test/CreatedAtRouteResultTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ private static IServiceProvider CreateServices()
services.AddSingleton<IActionResultExecutor<ObjectResult>>(new ObjectResultExecutor(
new DefaultOutputFormatterSelector(options, NullLoggerFactory.Instance),
new TestHttpResponseStreamWriterFactory(),
NullLoggerFactory.Instance));
NullLoggerFactory.Instance,
options));

return services.BuildServiceProvider();
}
Expand Down
Loading