Json Serializer ReferenceLoopHandling / Truncated response / Chunked terminator behaviour

[chrisckc]

Hi,

This is a continuation of an issue that i and others have previously raised with v1.x

aspnet/IISIntegration#285

#26

I wanted to re-test the behaviour of this so I upgraded a test project i originally created last year to demo the issue from dotnet v1.x to v2.0.0 (i am currently using SDK v2.0.3)

Previously in dotnet1.0:

Previously in v1 when an object graph containing a circular reference was serialized, the JSON response text was truncated at the point where the circular reference would have been encountered, but the JSON was correctly closed off with the correct curly braces and square bracket. The response was also missing the Chunked Terminator and showed an error indicating such in the Chrome developer console. However the response showed a 200 OK status code caused by the fact that the response headers had already been sent before the exception was encountered (due to the response not being buffered). When i added the ResponseBuffering middleware i managed to get a 500 status code and exception to be returned.

Now in dotnet2.0:

The behaviour in version 2 is slightly different, the status code is still 200 OK due to the unbuffered response, however the truncated JSON is now properly truncated and does not have the closing braces and square bracket etc. The Chunked terminator now seems to be present as i am no longer seeing an error in the Chrome developer console. Adding the ResponseBuffering middleware still allows a 500 status code and exception as previously.

As in v1 , using v2, if i set the serializer to ignore ReferenceLoop errors then it works fine because the self referencing loop is ignored rather than followed:
o.SerializerSettings.ReferenceLoopHandling = ReferenceLoopHandling.Ignore;

I have the following 4 thoughts on this:

1. The 200 status code is misleading but unavoidable due to the unbuffered response, if ultimate performance is not an issue then use the ResponseBuffering middleware to simplify client error handling behaviour.

2. If you are using EF, depending on your model you will probably end up having to use ReferenceLoopHandling.Ignore unless you want to write code to manipulate your object graph to remove any loops.

3. The truncated JSON is now correct in that it is no longer valid JSON so should prevent incorrect / misleading data from being unknowingly received by a client, the client's JSON parser should throw an error.

4. The Chunked terminator now being present could either be a bad thing or a good thing depending on your point of view or preference. In my opinion now that the JSON is invalid the chunked terminator may as well be present so at least the response will be valid and not confuse any reverse proxy's or client libraries.

So all in all this is an improvement, however... if i use the ResponseCompression middleware it ends up back in the land of the unknown because i now get a completely empty response body with the 200 OK status code, so a client may well think there is genuinely no data available from the request it just made, and won't know there was actually an error thrown on the server.

This is a good example of why having to rely on a quirk of behaviour to detect errors is bad, the behaviour can easily be changed by configuration changes, such as adding some middleware in this example. It seems like the only way make to make the behaviour consistent is to use response buffering, so i think this should really be the default configuration for a new .NET Core WebApi project.

The ResponseCompression middleware is working correctly in normal circumstances as i can see the response in both Postman and Chrome with the header Content-Encoding: gzip

Btw. The above was tested on a Mac using the AspNetCore Kestrel server that runs when i use "dotnet run"

I have now updated the demo repo which i used originally to dotnet2.0 and added the ResponseCompression middleware:
https://github.com/chrisckc/TodoApiDemo

[Tratcher]

This issue was moved to dotnet/aspnetcore#2285

[Tratcher]

RE: 4. I've run your repro and while I do see confusing behavior, I don't see an actual chunk terminator anywhere. The browsers appear to process and report errors differently for compressed responses. If you use a tool like Fiddler you'll always see an error for the missing terminator.