Repository navigation
Fix ArgumentNullException in per-request log buffering at the end of a request - #7811
Merged
evgenyfedorov2 merged 8 commits intoOct 6, 2026
Merged
evgenyfedorov2 merged 8 commits into
evgenyfedorov2 merged 8 commits into
Conversation
…a request PerRequestLogBufferManager read HttpContext.RequestServices without checking it for null. ASP.NET Core disposes the request services, after which RequestServices returns null, before it logs "Request finished" and clears IHttpContextAccessor.HttpContext. TryEnqueue now uses the global buffer in that case, and Flush skips the per-request buffer and still flushes the global buffer. Fixes #7806 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the test that reproduces the reported scenario ("Request finished"
logged after the request services have been disposed) and remove the unit
tests that used an HttpContext without request services.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Collaborator
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1624770&view=codecoverage-tab |
Replace the IServiceProvider? locals with ?. on HttpContext.RequestServices. TryEnqueue already falls back to the global buffer when there's no per-request buffer, and Flush still flushes the global buffer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use Assert.Contains on FakeLogCollector.GetLogsAsync instead of a loop, and remove the status code check, which the bug doesn't affect. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the end-to-end test with a test of PerRequestLogBufferManager: a record logged while HttpContext.RequestServices is null is held by the global buffer and written when PerRequestLogBuffer is flushed. The test covers both TryEnqueue and Flush. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused null handling matches the reported lifecycle behavior and is adequately covered by a regression test.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes per-request log buffering when request services have already been disposed.
Changes:
- Falls back to the global buffer when
RequestServicesis null. - Adds regression coverage for logging and flushing.
| File | Description |
|---|---|
PerRequestLogBufferManager.cs |
Safely handles null request services. |
PerRequestLogBufferManagerTests.cs |
Verifies global-buffer fallback behavior. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Collaborator
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1624977&view=codecoverage-tab |
ykumashev
approved these changes
Oct 6, 2026
evgenyfedorov2
pushed a commit
that referenced
this pull request
Oct 7, 2026
#7811 fixed the exception the filter avoided. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7806
Problem
As reported in #7806, with per-request log buffering (
AddPerIncomingRequestBuffer) and Information level enabled forMicrosoft.AspNetCore.Hosting.Diagnostics,ILogger.LogthrowsArgumentNullException: Value cannot be null. (Parameter 'provider')when ASP.NET Core logs "Request finished" (event id 2) at the end of a request.Root cause
ASP.NET Core disposes the request's service scope in a response
OnCompletedcallback, after whichHttpContext.RequestServicesreturnsnull, and then logs "Request finished" inHostingApplication.DisposeContextwhileIHttpContextAccessor.HttpContextstill returns the request'sHttpContext.PerRequestLogBufferManager.TryEnqueueandFlushonly checkedHttpContextfornull, so they passed thenullRequestServicestoGetService<T>(), which throws.ExtendedLoggercallsTryEnqueueoutside itstry/catch, so the exception escapesILogger.Log.Fix
In
PerRequestLogBufferManager:TryEnqueueuses the global buffer whenHttpContextor itsRequestServicesisnull, the same way it already handled anullHttpContext.Flushskips the per-request buffer whenRequestServicesisnulland still flushes the global buffer.