-
-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Failed to extract body - NullReferenceException #1701
Comments
can you share your code for |
so it seems most member of DefaultHttpRequest can throw a nullref if accessed concurrently dotnet/aspnetcore#42040 so we can handle that. However i am unable to work out why this would be an infinite loop, nor repro it. @mattjohnsonpint thoughts? |
Do we know why there's concurrent access? A null check isn't enough to fix it.
Concurrent access to dictionaries. |
@davidfowl at the moment no. the code in question is here https://github.com/getsentry/sentry-dotnet/tree/main/src/Sentry.AspNetCore which i am reviewing for this scenario. but you will notice the original stack trace does not occur from within sentry
So i am unsure if the sentry integration is causing this, or if it is just hitting some other underlying issue. @msxdan how often does this occur? |
not that i can find |
Turn off the sentry and see if it repros? There's a common pattern where this happens around usage of the |
yeah i was intending to ask @msxdan this based on the answer to "how often does this occur?" |
I'm working with @msxdan on this issue. I'll try to clarify some points.
From what I see in the log, those are written on disk through NLog, which seems to work and from what I figured out, then NLog sends the exception to the Sentry target that fails reading the I add this excerpt from the thread dump we captured in Azure, originally has more than 80K lines with the same over and over:
I hope this helps. |
Is this happening on windows? |
Azure AppService, but they run in windows |
That stack is helpful but the redaction makes it hard to figure out what the bottom of the stack is. Does this app use the IHttpContextAccessor anywhere in the logging stack? |
As addition to the first log, this is the log of the second time this happened Looks like after cancelling a task Sentry tries to catch the exception and enters an infinite loop again, this time the exception was thrown by As metioned above, we've disabled the
My teammate @mcliment knows better how it's done, he will reply asap |
I add the very beginning of the stack trace until it starst looping (cc @davidfowl):
|
@mcliment are you using any of nlog's extensiblity? |
This is a very good question... indeed we use |
I see that we have been using Thanks for your tips. |
This was removed outright in NLog.Web.AspNetCore 5.0 due to these issues. I added a PR to fix aspnet-request-posted-body for NLog.Web.AspNetCore 5.1, it requires you to install an ASP.NET Middleware class we added the NLog.Web.AspnetCore to capture the request, that way it is thread safe. I have been using that way to capture both the request and response body at my work for that past 2 years and have had no issues under load. Just remember to use it for requests only less than 30 KB in length. Above that ASP.NET Core HttpRequest.EnableBuffering() writes to TEMP files, according to its documentation. Therefore, our default request length above which we refuse to capture the request body is therefore 30 KB, but you could override that if you want to live dangerously. (I also added about a dozen more layout renderers for asp.net and asp.net core for the upcoming release). We are also adding an example to the wiki for response body capturing middleware. If you need it re-activated before the release I can show you how to add the request middleware class and log the request body to HttpContext.Items, then use one of the existing layout renders that prints an HttpContext.Item out. |
Sounds like this bug is only related to the NLog issues mentioned, so I'll close this issue. If I'm mistaken and there's something more you need Sentry to investigate, please let me know and I'll re-open. Thanks. |
Thanks to @bakgerman now NLog.Web.AspNetCore v5.1 re-introduces ${aspnet-request-posted-body} with help from middleware: app.UseMiddleware<NLog.Web.NLogRequestPostedBodyMiddleware>(); It is no longer necessary to explict call |
Package
Sentry.AspNetCore
.NET Flavor
.NET
.NET Version
6.0.201
OS
Windows
SDK Version
3.16.0
Self-Hosted Sentry Version
No response
Steps to Reproduce
We could not reproduce the issue, it happens randomly and seldomly but it makes the server crash
Expected Result
The error should be triggered only once
Actual Result
It seems that NLog logs properly but when sending the log to Sentry it enters in a infinite loop.
The text was updated successfully, but these errors were encountered: