Skip to content
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

[Discussion] Actions returning IAsyncEnumerable<> are no longer buffered by MVC when using System.Text.Json #32483

Closed
pranavkm opened this issue May 6, 2021 · 10 comments
Milestone

Comments

@pranavkm
Copy link
Contributor

pranavkm commented May 6, 2021

Actions returning IAsyncEnumerable<> are no longer buffered by MVC when using System.Text.Json

In 5.0, MVC added support for output formatting IAsyncEnumerable<> types by buffering the sequence in memory and formatting the buffered collection. In 6.0, when formatting using System.Text.Json, IAsyncEnumerable<> instances are no longer buffered by MVC, instead relying on the support for these types added to System.Text.Json.

In most cases, the absence of buffering would not be observed by the application. However, some scenarios may have inadvertently relied on the buffering semantics to correctly serialize. For instance, returning an IAsyncEnumerable<> that is backed by a EF query on a type with lazy loaded properties might result in concurrent query execution which might be unsupported by the provider.

This change does not affect output formatting using Newtonsoft.Json, or with XML-based formatters

Version introduced

6.0-preview4

Old behavior

IAsyncEnumerable<> instances returned from an MVC action as a value to be formatted using ObjectResult, or a JsonResult would be buffered before being serialized as synchronous collection.

New behavior

When formatting using System.Text.Json, IAsyncEnumerable<> instances are no longer buffered by MVC.

Reason for change

System.Text.Json added support for streaming IAsyncEnumerable<> types. This allows for a smaller memory footprint during serialization.

Recommended action

If your application requires buffering, consider manually buffering the async enumerable:

// Before
public IActionResult Get()
{
    return Ok(dbContext.Blogs);
}

// After
public async Task<IActionResult> Get()
{
    return Ok(await dbContext.Blogs.ToListAsync());
}
@jeffhandley
Copy link
Member

Just to link the issues together, #32254 covered a case where this breaking change surfaced in the .NET 6 Preview 4 build validation.

@mrgleba
Copy link

mrgleba commented May 26, 2021

How would exceptions be handled in this scenario.
For example after some data has elready been flushed to the client and some exception is thrown?

Another question: would the response body be flushed after every step of async enumeration or there would
be some internal buffer that gets filled and is sent when full?

@pranavkm
Copy link
Contributor Author

How would exceptions be handled in this scenario.

The behavior would be fairly similar to exceptions encountered part way thru serializing a JSON payload - the client would have already seen headers and possibly seen some of it's content. If you expect processing to possibly throw, consider materializing the IAsyncEnumerable to manually handle exceptions.

would the response body be flushed after every step of async enumeration or there would
be some internal buffer that gets filled and is sent when full?

@eiriktsarpalis, do you have more details about when STJ flushes when serializing an IAsyncEnumerable?

@mrgleba
Copy link

mrgleba commented May 27, 2021

The behavior would be fairly similar to exceptions encountered part way thru serializing a JSON payload - the client would have already seen headers and possibly seen some of it's content. If you expect processing to possibly throw, consider materializing the IAsyncEnumerable to manually handle exceptions.

You should always expect some code to throw. In this new scenario I can see no good way to pass exception information to the client (only trailers, which became widely supported recently, come to mind: https://caniuse.com/?search=trailer) . So you'll have to either to refactor all your code to materialze response before returning or loose exception handling in the client.

This is a big breaking change and introduces an unexpected, different behaviour, which could be very confusing to the person writing the code. All this in exchange for a improvement in memory perf.

Consider a junior programmer trying out aspnet core. He writes 3 actions:

public class Foo {
  public async IAsyncEnumerable<string>() Test1 {
     var ret = await ProcThatThrowsAsync();
     yield return ret;
  }

  public async IAsyncEnumerable<string>() Test2 {
     var ret = await ProcThatThrowsAtOne(0);
     yield return ret;
     ret = await ProcThatThrowsAtOne(1);
     yield return ret;
  }

  public async IAsyncEnumerable<string>() Test3 {
     var ret = await ProcThatThrowsAtTwo(0);
     yield return ret;
     ret = await ProcThatThrowsATwo(1);
     yield return ret;
     ret = await ProcThatThrowsATwo(2);
     yield return ret;
  }
}

The result they might see is:

  • http 500 in Test1 with error information
  • http 500 in Test2with error information (data yet buffering, nothing has been sent, so we can shape our response)
  • http 200 and broken stream in Test3 (status, headers and some data sent to the client before expcetion) - no obviuos way to handle this client-side.

There would be no obvious indication why Test2 behaves differently than Test3 unless they know where to look and understand how HTTP works.

IMHO sending data as chunked rather than whole should be opt-in (attribute for example) so there is backward compatibility and
the persown writing the code can see all the caveats in the docs before making the decision to change behaviour.

@davidfowl
Copy link
Member

Aren't exceptions logged server side? IAsyncEnumerable should have been streaming from the start and we're fixing that behavior. It is a breaking change but the workaround is simple if you want to buffer the response (which make sense for small finite sequences).

I think producing truly async sequences using yield is pretty advanced for web scenarios (happy to be proven wrong on that). In the EF case, you should have already been using ToListAsync (or some equivalent) to make sure exceptions were observed in your code rather than in the formatter code.

I agree this is a big breaking change but we think it's the right behavior and hopefully not mainstream enough to break the masses in new ways.

@eiriktsarpalis, do you have more details about when STJ flushes when serializing an IAsyncEnumerable?

It flushes at some buffer size limit that's internal to the JSON serializer.

@mrgleba
Copy link

mrgleba commented May 27, 2021

Aren't exceptions logged server side?

They are. I'm refering more to a SPA scenario.
After the change if something breaks, you'll end with a 200 status and a broken stream on the client
with no idea what happened. You'll have to either refactor the server code to buffer (tedious) or the client code to handle broken streams (potentially non-trivial).

IAsyncEnumerable should have been streaming from the start and we're fixing that behavior.

Great 😄

It is a breaking change but the workaround is simple if you want to buffer the response (which make sense for small finite sequences).

It is simple, but you have to do it and you have to know you have to do it (one more item on the entry learning path).
The opt-int would be event simpler and would prove it's what the users wants (and is aware of the costs).
Additionally opt-in could be also potentially extended to IEnumerable<> (and potentially other if the user cares for sending chunked) without another breaking change.

IMHO The change gains memory efficiency (and memory in most cases is cheap and abundant, no more 640kb should be enough for everyone these days 😄 ) at the sacrifice of consistency and developer experience (which are dear).

I think producing truly async sequences using yield is pretty advanced for web scenarios (happy to be proven wrong on that). In the EF case, you should have already been using ToListAsync (or some equivalent) to make sure exceptions were observed in your code rather than in the formatter code.

In our case we're using source generoators to transate SQL DDL into classes representing tables then we write queries in C# in SQL-like fashion

        public IQuery<(int grupaLiczId, DateTime dtOd, DateTime dtDo, int zaksiegowany)> LicznikiTerminRozl(int wId, int uId) => Query(q => q.
            From(ActiveWM(wId, 1, uId, 0, 0), out var wm).
                InnerJoin(C.ROZLLICZ, out var rozLicz).On(wm.id == rozLicz.WID && wm.id == wId).
            Select((rozLicz.GLID, rozLicz.DTOD, rozLicz.DTDO, IsNull(rozLicz.NTID) ? 0 : 1), out var sel).
            OrderBy(sel.GLID).
            OrderByDesc(sel.DTOD));

and then another source generator translates that into strongly typed access classes that produce IAsyncEnumerable<> with compiled SQL included. Most API endpoints are then just invocations of generated access classes and would need refactoring to adapt to this change.
Just to say - not everybody uses EF 😄

Thanks for hearing me out and caring to answer. I love and appreciate the wonderful work you're doing 🎉

@eiriktsarpalis
Copy link
Member

@eiriktsarpalis, do you have more details about when STJ flushes when serializing an IAsyncEnumerable?

It flushes at some buffer size limit that's internal to the JSON serializer.

That's right, and to add to that we're also flushing every time IAsyncEnumerator.MoveNextAsync() returns an incomplete task.

@pranavkm
Copy link
Contributor Author

As you pointed out, serializing IEnumerable doesn't currently have this behavior. It's also entirely possible that serializing POCOs throws because one of the property getters throws while the response headers have already been sent. If guaranteeing correctness of serialized behavior is desired, buffering the response is the only way to go rather than special casing specific types that are endemic to your app model.

@ghost
Copy link

ghost commented Jul 27, 2021

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!

@ghost
Copy link

ghost commented Sep 25, 2021

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!

@ghost ghost closed this as completed Sep 25, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants