[Json] Avoid writing to PipeWriter if IAsyncEnumerable throws before first item#113503
Conversation
| } | ||
| else | ||
| { | ||
| writer.Flush(); |
There was a problem hiding this comment.
Is this fixing an unrelated bug related to SuppressFlush handling?
There was a problem hiding this comment.
Nope, writer.Flush() calls Advance on the underlying PipeWriter which is what we're trying to avoid here. So without the move, [ would be written to the PipeWriter still before observing the exception from the IAsyncEnumerable.
| } | ||
| catch | ||
| { | ||
| // Reset the writer in exception cases as we don't want the writer.Dispose() call in the finally block to flush any pending bytes. |
There was a problem hiding this comment.
If we only do this to prevent the writer.Dispose() from flushing, why not just collocate the two calls?
| // Regression test for https://github.com/dotnet/aspnetcore/issues/36977 | ||
| using var stream = new MemoryStream(); | ||
| await Assert.ThrowsAsync<NotImplementedException>(async () => await StreamingSerializer.SerializeWrapper(stream, new AsyncEnumerableDto<int> { Data = GetFailingAsyncEnumerable() })); | ||
| await Assert.ThrowsAsync<NotImplementedException>(async () => await StreamingSerializer.SerializeWrapper(stream, GetFailingAsyncEnumerable())); |
There was a problem hiding this comment.
I don't recall what this regression test is checking, but why does this now need to be changed?
There was a problem hiding this comment.
The regression, at least from ASP.NET Core side, was a pure IAsyncEnumerable<T>, so wrapping it in a DTO seemed odd. It also was initially failing in the Pipe case as the "Data:" bit was being flushed, but it seems I can reset the test now that I've moved the writer.Flush() call into the SuppressFlush check if we'd like.
|
Updated |
|
/backport to release/9.0-staging |
|
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/13952382257 |
|
Was this this a regression from .NET 8? |
|
The PipeWriter overload is new in 9.0 |
|
Then I'm not sure if it meets the bar for servicing. |
|
While technically not a regression from STJ perspective since it is a new API, it is a regression from ASP.NET Core perspective since we changed to use the new API in 9.0. |
|
Did I not already do that? |
|
The reproduction is an STJ snippet. Given that we're contemplating a backport on the grounds of an ASP.NET regression I would suggest putting all emphasis on that. |
|
Sure, updated with more emphasis. |
|
Thanks |
2 participants
Resolves dotnet/aspnetcore#60911