Missing async versions for Bind<T> and BindAndValidate<T>

[cartercommunity]

Missing async versions for Bind and BindAndValidate

from carter.

[davidfowl]

Memory stream is a bad idea that’s why we don’t do that 🙂.

[jchannon]

Ah good spot! Thanks. For asp.net core 3 we will probably move to the new json serializer that comes with it

…

On Tue, 16 Jul 2019 at 10:17, Stephen Pope ***@***.***> wrote: This seemed the most logical place to put this .. I can open up a seperate issue if needed. I was starting to use Carter (4.2.0) with Netcore 3.0 Preview 6 (3.0.100-preview6-12264). When trying to call BindAndValidate I get the following error .. System.InvalidOperationException: Synchronous operations are disallowed. Call ReadAsync or set AllowSynchronousIO to true instead. at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpRequestStream.Read(Byte[] buffer, Int32 offset, Int32 count) at System.IO.StreamReader.ReadBuffer(Span`1 userBuffer, Boolean& readToUserBuffer) at System.IO.StreamReader.ReadSpan(Span`1 buffer) at System.IO.StreamReader.Read(Char[] buffer, Int32 index, Int32 count) at Newtonsoft.Json.JsonTextReader.ReadData(Boolean append, Int32 charsRequired) at Newtonsoft.Json.JsonTextReader.ParseValue() at Newtonsoft.Json.JsonReader.ReadAndMoveToContent() at Newtonsoft.Json.JsonReader.ReadForType(JsonContract contract, Boolean hasConverter) at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent) at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType) at Newtonsoft.Json.JsonSerializer.Deserialize(JsonReader reader, Type objectType) at Newtonsoft.Json.JsonSerializer.Deserialize[T](JsonReader reader) at Carter.ModelBinding.BindExtensions.Bind[T](HttpRequest request) at Carter.ModelBinding.BindExtensions.BindAndValidate[T](HttpRequest request) It seems to relate to this announcement -> dotnet/aspnetcore#7644 <dotnet/aspnetcore#7644> .. that synchronousIO is disallowed by default in NetCore 3. Adding : var syncIOFeature = req.HttpContext.Features.Get<IHttpBodyControlFeature>(); if (syncIOFeature != null) { syncIOFeature.AllowSynchronousIO = true; } .. before calling it solves the issue. But it seems that BindAndValidate method is going to blow up by default from NetCore 3 onwards. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#39?email_source=notifications&email_token=AAAZVJU4WTIQFAXBJYIEXVTP7WGY3A5CNFSM4D7HB4W2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2AHS4Q#issuecomment-511736178>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAZVJR32VCYGBNDMHJH6CLP7WGY3ANCNFSM4D7HB4WQ> .

[jchannon]

I don't really know JSON.Net that well in all honesty and dislike the dependency generally due to binding redirect hell 😄 however I took that decision took a dependency on it to get going ASAP. I like the idea of a async BindAndValidate though, just not sure how we do that looking at the link you gave, as it would load the body into a JToken, JArray etc then I assume we have to convert from that to the model. I'm confused why they didn't make their JsonSerialzier.Deserialize method async.

I'm also tempted to look at this new library and dropping the Json.Net dependency https://github.com/neuecc/Utf8Json/

Thoughts?

[fdipuma]

I understand your concerns on taking a dependency on Json.NET, but I do not really believe moving to another serialization library will improve much this issue.

From what I understand Json.NET is the de-facto standard when talking about Json serialization on .NET (which means the probability to already have it as a dependency inside the project is high).

While I do not fully agree on the decision of some team (see ASP.NET MVC Core team) to take this dependency as immutable, I really believe that if we want to fully be dependency-less we should somewhat mimic what Nancy already done: an internal basic Json serializer (similar to SimpleJson with an already implemented class), with pluggable alternative serializers.

Something like (stubbed):

public interface IModelBinderSerializer
{
    void Serialize<T>(T value, Stream stream);
    Task SerializeAsync<T>(T value, Stream stream);    
    T Deserialize<T>(Stream stream);
    Task<T> DeserializeAsync<T>(Stream stream);
}

This will certainly remove any direct dependency on serializers. We could then add different packages for Json.NET, Utf8Json, Whatever.

Regarding the Json.NET approach (assuming we want to keep it as a dependency) I was thinking to something simple and straightforward:

public static async Task<T> BindAsync<T>(this HttpRequest request, CancellationToken cancellationToken = default(CancellationToken))
{
    JToken token;
    using (var streamReader = new StreamReader(request.Body))
    using (var jsonTextReader = new JsonTextReader(streamReader))
    {
        token = await JToken.LoadAsync(jsonTextReader, cancellationToken);
    }
    return token.ToObject<T>();
}

I do not really know why they do not implement a friendly JsonConvert.DeserializeAsync method, but I understand that now this is the only approach for async based deserialization.

Looking forward for your thoughts!

[jchannon]

Whilst I think the stub is fine to make it like Nancy, for now I think I want to keep Botwin "opinionated" but performant.

I'd like to find out if swapping JsonConvert.Deserialize to JToken.LoadAsync is more performant somehow.

UPDATE Looking at decompiled DeserializeObject it uses the StreamReader & JsonTextReader but its not async

[jchannon]

@davidfowl any thoughts?

[davidfowl]

Whilst I think the stub is fine to make it like Nancy, for now I think I want to keep Botwin "opinionated" but performant.

Do you have any performance tests?

If Bind is doing synchronous IO then users of Botwin will have a scalability issue, to mitigate that in MVC currently is this what the JsonInputFormatter does:

https://github.com/aspnet/Mvc/blob/0989e60f735eabfba71b6db6e08cfa7f84c329ac/src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonInputFormatter.cs#L149-L150

The goal is to move away from this and use the newer JSON.NET so that we can do true async reads. You can use a similar approach if the JSON serializer doesn't support async reads.

[jchannon]

Guess I should run wrk on both approaches and check the results 😄

[jchannon]

I added the below code to load the json async and validate the model async and the wrk results are inconcusive 😕

        public static async Task<(ValidationResult ValidationResult, T Data)> BindAndValidateAsync<T>(this HttpRequest request)
        {
            var data = await request.BindAsync<T>();
            if (data == null)
            {
                data = Activator.CreateInstance<T>();
            }

            var validatorLocator = request.HttpContext.RequestServices.GetService<IValidatorLocator>();

            var validator = validatorLocator.GetValidator<T>();

            if (validator == null)
            {
                return (new ValidationResult(new[] { new ValidationFailure(typeof(T).Name, "No validator found") }), default(T));
            }

            var result = await validator.ValidateAsync(data);
            return (result, data);
        }
        
        public static async Task<T> BindAsync<T>(this HttpRequest request, CancellationToken cancellationToken = default(CancellationToken))
        {
            JToken token;
            using (var streamReader = new StreamReader(request.Body))
            using (var jsonTextReader = new JsonTextReader(streamReader))
            {
                token = await JToken.LoadAsync(jsonTextReader, cancellationToken);
            }
            return token.ToObject<T>();
        }

[jchannon]

@JamesNK does the above code look right to read a request body and deserialize async?

[jchannon]

Used the same async code above with https://github.com/neuecc/Utf8Json/ and again results are no different. I wonder if I'm not stressing the fwk hard enough and each approach is handling fine at the levels the test is running at

[fdipuma]

Can you share some of the results and testing profile you used?

[jchannon]

Using wrk I did 4 threads 100 connections for 10seconds. Results were roughly 8-9k req/s

[JamesNK]

It would probably be cheaper to load a copy of the request body into a MemoryStream than a JToken. JToken has various internal references as well as internal dictionaries to speed up fetching properties by name, but takes up more memory.

[jchannon]

@JamesNK like this? https://github.com/jchannon/Botwin/blob/master/src/ModelBinding/BindExtensions.cs#L40

[fdipuma]

A quick-and-dirty solution may be something like this:

public static T Bind<T>(this HttpRequest request)
{
    return request.Body.BindFromStream<T>();
}

private const int BufferSize = 1024 * 4;

public static async Task<T> BindAsync<T>(this HttpRequest request, CancellationToken cancellationToken = default(CancellationToken))
{
    using(var ms = new MemoryStream())
    {
        await request.Body.CopyToAsync(ms, BufferSize, cancellationToken);
        ms.Position = 0;
        return ms.BindFromStream<T>();
    }    
}

private static T BindFromStream<T>(this Stream stream)
{
    using (var streamReader = new StreamReader(stream))
    using (var jsonTextReader = new JsonTextReader(streamReader))
    {
        return JsonSerializer.Deserialize<T>(jsonTextReader);
    }
}

I believe a more efficient approach should leverage the use of ArrayPool and stuff as @davidfowl has shown (similar to what ASP.NET MVC team has already done).

[stephenpope]

This seemed the most logical place to put this .. I can open up a seperate issue if needed.

I was starting to use Carter (4.2.0) with Netcore 3.0 Preview 6 (3.0.100-preview6-12264).

When trying to call BindAndValidate I get the following error ..

System.InvalidOperationException: Synchronous operations are disallowed. Call ReadAsync or set AllowSynchronousIO to true instead.
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpRequestStream.Read(Byte[] buffer, Int32 offset, Int32 count)
   at System.IO.StreamReader.ReadBuffer(Span`1 userBuffer, Boolean& readToUserBuffer)
   at System.IO.StreamReader.ReadSpan(Span`1 buffer)
   at System.IO.StreamReader.Read(Char[] buffer, Int32 index, Int32 count)
   at Newtonsoft.Json.JsonTextReader.ReadData(Boolean append, Int32 charsRequired)
   at Newtonsoft.Json.JsonTextReader.ParseValue()
   at Newtonsoft.Json.JsonReader.ReadAndMoveToContent()
   at Newtonsoft.Json.JsonReader.ReadForType(JsonContract contract, Boolean hasConverter)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonSerializer.Deserialize(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonSerializer.Deserialize[T](JsonReader reader)
   at Carter.ModelBinding.BindExtensions.Bind[T](HttpRequest request)
   at Carter.ModelBinding.BindExtensions.BindAndValidate[T](HttpRequest request)

It seems to relate to this announcement -> dotnet/aspnetcore#7644 .. that synchronousIO is disallowed by default in NetCore 3.

Adding :

var syncIOFeature = req.HttpContext.Features.Get<IHttpBodyControlFeature>();
                
if (syncIOFeature != null)
{
    syncIOFeature.AllowSynchronousIO = true;
}

.. before calling it solves the issue. But it seems that BindAndValidate method is going to blow up by default from NetCore 3 onwards.

[danielmoncada]

@jchannon, has there been an update on a BindAsync? Currently migrating to ASP.NET Core 3, and am looking for a solution. Performance wise, not sure where to go, as @JamesNK indicates to use a MemoryStream, but @davidfowl states that it's a bad idea. @fdipuma curious on which route you took here.

Also note that JSON.Net is a must for us at the moment, as System.Text.Json does not support dyanmic/expando types.

[jchannon]

The aspnetcore 3 branch is async only but it uses the new serializer

…

On Tue, 15 Oct 2019 at 23:13, Daniel Moncada ***@***.***> wrote: @jchannon <https://github.com/jchannon>, has there been an update on a BindAsync? Currently migrating to ASP.NET Core 3, and am looking for a solution. Performance wise, not sure where to go, as @JamesNK <https://github.com/JamesNK> indicates to use a MemoryStream, but @davidfowl <https://github.com/davidfowl> states that it's a bad idea. @fdipuma <https://github.com/fdipuma> curious on which route you took here. Also note that JSON.Net is a must for us at the moment, as System.Text.Json does not support dyanmic/expando types. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39?email_source=notifications&email_token=AAAZVJWAURWOE7XWFP6HYSDQOY6BHA5CNFSM4D7HB4W2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBKMSQA#issuecomment-542427456>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZVJU7LPBZTVBGOLOCLG3QOY6BHANCNFSM4D7HB4WQ> .