[Json] Avoid writing to PipeWriter if IAsyncEnumerable throws before first item #113503
Conversation
| @@ -179,6 +178,7 @@ rootValue is not null && | |||
| } | |||
| 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.
| @@ -230,6 +230,8 @@ rootValue is not null && | |||
| } | |||
| 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?
| @@ -354,7 +354,7 @@ public async Task RegressionTest_ExceptionOnFirstMoveNextShouldNotFlushBuffer() | |||
|
|
|||
| // 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 |
Resolves dotnet/aspnetcore#60911