Use System.Text.Json's IAsyncEnumerable support #31894
Conversation
ghost
added
the
area-mvc
dotnet-maestro
bot
requested review from
dougbu,
halter73,
jkotalik,
Tratcher and
a team
as code owners
pranavkm
force-pushed
the
prkrishn/asyncenumerable
branch
from
846fd6f to
17ae3b8
Compare
pranavkm
force-pushed
the
prkrishn/asyncenumerable
branch
from
17ae3b8 to
5d4b871
Compare
| } | ||
| } | ||
| // Removed Log. | ||
| // new EventId(1, "BufferingAsyncEnumerable") |
|
This PR description should say something like: IAsyncEnumerable is a serializer concern, not an MVC concern 😄. This change looks good. Can we get this into preview4? |
pranavkm
force-pushed
the
prkrishn/asyncenumerable
branch
from
5d4b871 to
ed0af15
Compare
…msdk (#31748) [main] Update dependencies from dotnet/efcore dotnet/runtime dotnet/emsdk - Update to a newer SDK - Skip uninstall step - Update NuGet version to keep up with the SDK - Fixup test - Bumping SDK in attempt to fix `dotnet new` issues - Try only 3 platforms - Try assembly info - using - Quarantine test
| @@ -3,6 +3,7 @@ | |||
| <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework> | |||
| <RuntimeIdentifier>browser-wasm</RuntimeIdentifier> | |||
| <SelfContained>true</SelfContained> | |||
| <UseMonoRuntime>true</UseMonoRuntime> | |||
There was a problem hiding this comment.
Is this really related to this PR, or just something we need for another reason?
Could you clarify why we need it (and didn't before)?
There was a problem hiding this comment.
This change was ported from the PR in main: https://github.com/dotnet/aspnetcore/pull/31748/files. The targeting packs in the runtime were recently renamed and this is now required if you do not reference the BlazorWebAssembly SDK (which this project does not)
| var enumerated = await reader(asyncEnumerable); | ||
| await ExecuteAsyncCore(context, result, enumerated.GetType(), enumerated); | ||
| } | ||
|
|
There was a problem hiding this comment.
By doing this, are we not removing support for this on other formatters? (JSON.NET, XML)
Nevermind, I see that this has been pushed down to our two other formatters. That said, this might break people creating their own formatter?
Would it make sense for this to remain on the base class and have STJ formatter signal that it doesn't need it?
There was a problem hiding this comment.
That was one of the options I considered. The current implementation is also a bit of a whack-a-mole in that it only works for all the result types that we account for. Custom result types would have the same issue (e.g. if a user makes a XmlResult or a ProtobufResult). Given the choice of either handling it at per-formatter vs per-result, the latter seems like a better choice.
I can make an announcement since this is a breaking behavior change.
There was a problem hiding this comment.
Result types derived from ObjectResult would have gotten this for free right?
There was a problem hiding this comment.
In the ordinary case, yes. It's possible that there's also custom result executors that would negate it though.
There was a problem hiding this comment.
We have functional tests as part of https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/test/Mvc.FunctionalTests/AsyncEnumerableTestBase.cs as well as some integration tests for Json.
| var enumerated = await reader(asyncEnumerable); | ||
| await ExecuteAsyncCore(context, result, enumerated.GetType(), enumerated); | ||
| } | ||
|
|
There was a problem hiding this comment.
That was one of the options I considered. The current implementation is also a bit of a whack-a-mole in that it only works for all the result types that we account for. Custom result types would have the same issue (e.g. if a user makes a XmlResult or a ProtobufResult). Given the choice of either handling it at per-formatter vs per-result, the latter seems like a better choice.
I can make an announcement since this is a breaking behavior change.
pranavkm
added
the
Servicing-consider
|
Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge. |
davidfowl
added
the
breaking-change
mkArtakMSFT
added
Servicing-approved
|
Approved by tactics for preview4. |
|
@pranavkm is this ready to merge? let me know if you need help with merging. |
|
Yup, go for it. |
Description
System.Text.Json recently added support for streaming IAsyncEnumerable types based on an ask from the ASP.NET Core team. This PR enables MVC to leverage this feature.
Customer Impact
Using IAsyncEnumerable with System.Text.Json will result in data to be streamed rather than buffered and serialized.
Regression?
[If yes, specify the version the behavior has regressed from]
Risk
[Justify the selection above]
The feature has been well-tested in the runtime. This is simply removing additional buffering that was previously performed by MVC before invoking the serializer.
Verification
Packaging changes reviewed?
Addresses #11558 #23203