Fix HttpLoggingMiddleware Request/Response bodies logging in case of stream being closed by a subsequent middleware #61490
Conversation
…stream being closed by a subsequent middleware (dotnet#61489)
ghost
added
the
area-middleware
dotnet-policy-service
bot
added
the
community-contribution
|
Thanks for your PR, @@ExtraClock. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
@dotnet-policy-service agree |
dotnet-policy-service
bot
added
the
pending-ci-rerun
|
Hi @BrennanConroy ! Is there a chance for this PR to make it into the v10 release? Since it's my first PR, I'm not sure if there is any additional action I have to take to move this thing forward. |
|
|
||
| public override void Close() | ||
| { | ||
| if (!HasLogged) |
There was a problem hiding this comment.
Can we pass HttpLoggingInterceptorContext into the ctor like we do for the response stream and then check if body logging is enabled before getting the string?
| return result; | ||
| } | ||
|
|
||
| public override void Close() |
There was a problem hiding this comment.
Stream.Dispose() calls Close() which could then re-create the string just to throw it away. Maybe we should set _logBody to false once one of the LogResponseBody() methods is called.
I think the request stream impl is fine, but we should double-check and maybe add a comment about the assumption.
| // (1) The subsequent middleware reads right up to the buffer size (guided by the ContentLength header) | ||
| while (contentLengthBytesLeft > 0) | ||
| { | ||
| var res = await c.Request.Body.ReadAsync(arr, 0, (int)Math.Min(arr.Length, contentLengthBytesLeft)); |
There was a problem hiding this comment.
nit?
| var res = await c.Request.Body.ReadAsync(arr, 0, (int)Math.Min(arr.Length, contentLengthBytesLeft)); | |
| var res = await c.Request.Body.ReadAsync(arr, 0, arr.Length); |
|
|
||
| Assert.Contains(TestSink.Writes, w => w.Message.Contains(expected)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Could we add a test for options.CombineLogs = true and both Request and Response?
Fix HttpLoggingMiddleware Request/Response bodies logging in case of stream being closed by a subsequent middleware (#61489)
Preserve buffer contents until it is consumed by the
HttpLoggingMiddlewareDescription
Fixes #61489
When the subsequent middleware closes request/response stream, the
BufferingStreamcallsResetwhich leads to an empty body being logged.The request stream might be closed if the subsequent middleware follows the
ContentLengthheader to sense the end of the stream (instead of reading it to the end).The response stream might be closed without any additional preconditions, just because the subsequent middleware decided to do so.
A real-life example of such a middleware is
CoreWCFBasicHttpBinding: inputStream.Close, _outputStream.Close