JsonOutputFormatter adds all closing brackets when exceptions are thrown

[Tratcher]

RE: aspnet/IISIntegration#285
When there are exceptions thrown part way through object serialization the output incorrectly closes out all of the }, making the serialized result appear complete/valid rather than truncated.

Repro (reduced from JsonOutputFormatter):

    public class Program
    {
        public static void Main(string[] args)
        {
            var stream = new MemoryStream();
            try
            {
                using (var textWriter = new StreamWriter(stream, Encoding.UTF8, 1024, leaveOpen: true))
                {
                    // var jsonWriter = new JsonTextWriter(textWriter) { CloseOutput = false };
                    using (var jsonWriter = new JsonTextWriter(textWriter) { CloseOutput = false })
                    {
                        var serializer = JsonSerializer.Create();
                        serializer.Serialize(jsonWriter, new Foo());
                    }
                    // jsonWriter.Close();
                }
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex);
            }
            stream.Seek(0, SeekOrigin.Begin);
            var reader = new StreamReader(stream);
            Console.WriteLine(reader.ReadToEnd());
        }
    }

    public class Foo
    {
        public string A { get; } = "A";
        public string B
        {
            get
            {
                // return "B";
                throw new NotImplementedException("get_B");
            }
        }
    }

Expected output:
{"A":"A"

Actual output:
{"A":"A"}

Recommended fix:
Remove the using statement around the JsonTextWriter and only call Close on it if the serialization completes successfully.

[gdoron]

@Tratcher

Recommended fix:
Remove the using statement around the JsonTextWriter and only call Close on it if the serialization completes successfully

Won't you have unmanaged resourced just hanging out there un-disposed then?

[Tratcher]

@gdoron I inspected JsonTextWriter and it doesn't use any unmanaged resources, and even if it did it should use SafeHandles to track/clean them.

[gdoron]

@Tratcher I hate when classes without unmanaged resources implement IDisposable!
Thanks.

[rynowak]

@JamesNK any thoughts on this issue?

[JamesNK]

It's dumb behavior that I wrote about 10 years ago and I'd love to change it, but I've always worried about people who unknowingly relying on it being broken by the change.

[Eilon]

Oh dear. For MVC it'd also be a breaking change to stop doing this.

Could Json.NET have an option for this that could be configured through MVC's JSON configuration options? E.g. JsonSerializerSettings or whatnot?

[gdoron]

@JamesNK @Eilon Why not simply announce it as a breaking change on both JSON.NET and MVC, just in case someone is relying on the "dumb behavior"?
I think it's a much better option than carrying a "dumb" parameter forever which makes JSON.NET behave is it should by default, don't you think?

[Tratcher]

It's not even obvious how you would change Json.Net for this, the JsonTextWriter doesn't know an exception has occurred, only that the using block has closed. The proposal above does not require changes to Json.Net.

[Eilon]

@gdoron we take breaking changes very seriously and it's not clear what the benefit is of making this breaking change compared to leaving it as-is.

[gdoron]

Leaving it as is means most people will still have the existing behavior-bug...
Even if you will add an announcement, most developers won't see it.
And if you rely on people reading the announcement why not making it straightforward and make a way to fallback to the "dumb" behavior?

Just my two cents.

[Eilon]

@gdoron thanks, we'll definitely consider this, but the bar for breaking changes is very high. We just want to make sure we make the right decision here... because if we don't, we'll have to make another breaking change to undo it 😄

[chrisckc]

@Tratcher @JamesNK @Eilon @gdoron What i have found on a Mac using Kestrel is that when there are exceptions thrown part way through object serialization, (unless i use the AspNetCore.Buffering middleware) i receive a 200 OK response with valid Json, but missing the remaining data and the chunked terminator.

If this change were made (brackets left open) then i would still get a 200 OK, but with invalid Json and a missing chunked terminator, so still a broken response, just broken in a slightly different way.

Basically what i am saying is that this change wouldn't solve the main issue of the 200 OK status code, it does not solve the original issue that started this thread which is now here:
aspnet/IISIntegration#285

[Tratcher]

@chrisckc The 200 can only be changed by buffering the serialized content. Changing the serializer to avoid closing the brackets is one way to make the error more discoverable. Fixing aspnet/IISIntegration#39 is another way, but will only help some clients.

[halter73]

Fixing aspnet/IISIntegration#39 would at least fix the problem in Google Chrome and PowerShell's Invoke-WebRequest based on the investigation I did for aspnet/IISIntegration#285.

Are you aware of any client that would accept a response with a missing chunked terminator? That would be surprising.

[chrisckc]

@Tratcher Understood, and i agree that not closing off the brackets and so making the Json invalid will make the error easier to discover and prevent client libraries and dev tools such as postman from interpreting the response and completely successful (clients that are not bothered about the missing chunked terminator)

@halter73 I have yet to test some clients for the effect of the chunked terminator, ill get onto it soon.

[Tratcher]

@halter73 I'm not aware of any, but parsers that reach the end of the data struct (e.g. closing brackets) may not continue reading to notice the missing terminator.

[chrisckc]

Ok, i have tried AspNet WebApi Client and found that it correctly throws an exception with the missing chunked terminator

System.Net.Http.HttpRequestException: Error while copying content to a stream. ---> System.IO.IOException: The read operation failed, see inner exception. ---> System.Net.Http.CurlException: Transferred a partial file
   at System.Net.Http.CurlHandler.ThrowIfCURLEError(CURLcode error)

If I try curl from the terminal i get: curl: (18) transfer closed with outstanding read data remaining

The client repo is here: https://github.com/chrisckc/TodoApiClientDemo
Note that i had issues getting "Microsoft.AspNet.WebApi.Client" to work, i had to add a missing dependency "System.Runtime.Serialization.Xml" to avoid the exception: Could not load file or assembly 'System.Runtime.Serialization.Xml, Version=4.0.0.0 (that was when testing a valid response where it tries to read the content)

The above demo client points to my other demo repo that demonstrates the serialization issue: https://github.com/chrisckc/TodoApiDemo

Ill check my favourite iOS client library later.

[Tratcher]

I'm testing a fix for aspnet/IISIntegration#39 and found that Chrome has stopped showing an error on the page for truncated responses, they are only shown in the dev console. That makes fixing this issue more important. IE still shows an error for truncated content.

[halter73]

Does chrome actually show the truncated content though. In my testing, Chrome showed a blank page when hitting Kestrel directly, but nothing when going through IIS (along with the error in the dev console).

dotnet/efcore#6684 (comment)

[Tratcher]

It's showing truncated content now. Note I only tested text/plain.

[Tratcher]

It also shows the truncated content for text/html and applicaiton/json. I can only hope the JS APIs are better behaved.

[Tratcher]

Hit again: dotnet/aspnetcore#1834

[chrisckc]

@Tratcher Ok i have done some testing with various clients against my .Net Core WebApi method which creates the serialisation exception, and always returns a 200 status despite the exception occurring (due to the chucked transfer mechanics), here are the results:

Test1 (plain old Chrome):
As @halter73 pointed out, hitting the url using Chrome (tried with 54.0.2840.98 (64-bit) and 55.0.2883.75 (64-bit)) I get a blank page and can only see the "net::ERR_INCOMPLETE_CHUNKED_ENCODING" in the developer console as a clue to the issue.
Setting the response "Accept" header to "application/json" made no difference to Chrome's behaviour.

Test2 (Javascript Fetch):
Using the JavaScript "Fetch" Api an error is thrown: "Name: TypeError Message: Failed to fetch" , apparently that error message relates to an network error. I also see the "net::ERR_INCOMPLETE_CHUNKED_ENCODING" error is shown in the Chrome console but i am unable to access that error detail from the Javascript Api.

Test3 (iOS):
In an iOS App using the AFNetworking 3.0 library i get an error: "kCFErrorDomainCFNetwork: The network connection was lost" and I am not able access the response body. AFNetworking 3.0 is built on top of NSURLSession which uses CFNetwork so i expect the behaviour would be the same using that.

Test4 (Xamarin AndroidClientHandler):
In an Xamarin Android application, using HttpClient and setting the implementation to: "AndroidClientHandler" i get an exception generated from the _client.GetAsync(url) method
"Exception of type 'Java.IO.EOFException' was thrown. at System.Runtime.Excep…" so no access to the response body possible because GetAsync does not return. As this is using the Java classes i would expect the same behaviour in a native Java android App.
Test4.1 (Xamarin Android Managed handler)
However setting the implementation to "Managed (HttpClientHandler)", the request is successful, the _client.GetAsync(url) method returns and i am able to call response.Content.ReadAsAsync() and get an object back, therefore there is no way for the client to know that an error has occurred.

Summary
As can be seen the behaviour of clients cannot be relied upon to detect the error, so using chucked transfer would not be a good idea, suggesting that using ResponseBuffering middleware should be the default and users can disable it if they are aware of and accept the risks.

@Eilon as for preventing the JSON from being closed out when an exception occurs, this would solve the issue for Test4.1 above because response.Content.ReadAsAsync() would fail with an exception. But it would still end up causing confusion for consumers of an Api as the error would not point to the real issue unless you were fully aware of how the backend WebApi works and is configured etc.

[chrisckc]

@Tratcher @halter73 @Eilon I have just edit this post because i have realised what the issue was.

I was re-checking what i found previously using HttpClient in a .Net Core console App:

Ok, i have tried AspNet WebApi Client and found that it correctly throws an exception with the missing chunked terminator

I have since updated to the latest .Net Core a few days ago on my Mac (dotnet --version = 1.0.0-preview3-003221) and the behaviour from a console App is the same, i thought it had changed but i had made a typo in the url when testing.

One observation to make is that if the behaviour of curl changes in future OSX versions it would result in a change of behaviour of the HttpClient on OSX as it is clearly using curl under the hood from what i observed when i first tried.

At least the exception you get on OSX (and Linux i assume would use curl?) is better than what i see on Windows, using HttpClient on OSX i get this, notice "Transferred a partial file" which points to the issue:

System.Net.Http.HttpRequestException: Error while copying content to a stream. ---> System.IO.IOException: The read operation failed, see inner exception. ---> System.Net.Http.CurlException: Transferred a partial file
   at System.Net.Http.CurlHandler.ThrowIfCURLEError(CURLcode error)

On windows i get this which does not point to the issue (in usual windows style i'm afraid to say...)

Status: Exception: System.Net.Http.HttpRequestException: Error while copying con
tent to a stream. ---> System.IO.IOException: The read operation failed, see inn
er exception. ---> System.Net.Http.WinHttpException: The server returned an inva
lid or unrecognized response

[chrisckc]

I have created a RequestLogging middleware and whilst testing it I have found a way to capture serialization errors by wrapping the call to the next middleware "await _next.Invoke(context)" in a try catch block. It allows the errors to be logged on the backend which is better than nothing.

public async Task Invoke(HttpContext context)
{
	//start a Stopwatch and Log the Request
	//......
	
	// Setup the OnStarting callback so that we stop the stopwatch when the response starts to be sent to the client
	context.Response.OnStarting((state) =>
	{
		//Stop the stopwatch and record the elapsed time
		//......           
		return Task.FromResult(0);
	}, null);

	// Instead we can stop the timer after the response has finished being sent to the client is we want to ...
	// but i am not interested in including the network latency
	// context.Response.OnCompleted(() =>
	// {
	//     //Stop the stopwatch and record the elapsed time
		   //......
	//     return Task.FromResult(0);
	// });

	string errorMessage = null;
	try
	{
		//Wait for the rest of the Middlewares to complete
		await _next.Invoke(context);
	}
	catch (Exception ex)
	{
		
		//Capture the Error here then throw
		//......
		throw ex;
	}
	finally
	{
		//Log the Response and any Error and then tidy up
	}
}

I was curious to see if i could append some text to the response body using middleware by using code such as this after await _next.Invoke(context);

// Maybe try to write the error text to the end of the body
// This is not going to work because the connection will be probably be closed by now
//byte[] data = System.Text.Encoding.UTF8.GetBytes(errorMessage);
//await context.Response.Body.WriteAsync(data, 0, data.Length);

It had no effect, I also tried it in context.Response.OnCompleted(() and it has no effect.

[chrisckc]

@Tratcher @halter73 @Eilon I have done some further testing with HttpClient and found something interesting, i thought if the backend can use unbuffered responses then why not make the client unbuffered and return the response as soon as the headers have been received.

So i changed the code in my .Net Core test client to this:
response = await client.GetAsync(resourcePath, HttpCompletionOption.ResponseHeadersRead);
which according to the docs gives me the response after the headers have been received, the body is then transferred when i call this:
dynamic apiResponse = await response.Content.ReadAsAsync<dynamic>();
which is where i expected to see:
System.Net.Http.HttpRequestException: Error while copying content to a stream. ---> Transferred a partial file
However there was no error, the ReadAsAsync is successful and i get a valid object back, the status code is 200 as always and therefore there is no way for the client to know that an error has occurred. (this happens on Both OSX and Windows)

@Tratcher I can only hope the JS APIs are better behaved. Well it seems .Net's own client is not always well behaved. :-o

Btw. if i use
string responseBody = await response.Content.ReadAsStringAsync();
it behaves correctly and throws the error.
I realise that ReadAsAsync is part of the WebApi Client extension methods so looks like something suspect is going inside those.

I updated the repo with this new code:
https://github.com/chrisckc/TodoApiClientDemo

[JamesNK]

There is a new property on JsonWriter to control this - JamesNK/Newtonsoft.Json@55afb9a - and it is testable in this pre-release package - https://www.nuget.org/packages/Newtonsoft.Json/9.0.2-beta2

[Eilon]

@JamesNK super awesome, thanks for adding the feature!

I suspect that in ASP.NET Core 2.0 we'll want to enable that feature by default, so once there's a public "RTM" build of 9.0.2 I think we'll want to take a dependency on that.

[Tratcher]

@kichalla can you try out the beta and see if it gets us the the results we want?

[kichalla]

@Tratcher I verified the changes and they are good.

Thanks! @JamesNK

[kichalla]

A new release version of Json.net was made available today. Thanks @JamesNK
https://www.nuget.org/packages/Newtonsoft.Json/10.0.1

[kichalla]

I was trying to write some tests in MVC with the new JSON.Net package but noticed that the types BsonReader and BsonWriter have been moved to its own package (but renamed to BsonDataReader and BsonDataWriter), but this package is still in pre-release which means we cannot use it yet.

[kichalla]

FYI: BSON package was made out of pre-release today. Thanks @JamesNK