Skip to content

Don't capture HttpContext into the log objects #11723

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

Closed
wants to merge 1 commit into from

Conversation

benaadams
Copy link
Member

Don't capture HttpContext into the log objects from #10319

Resolves #10556
Resolves #10557

/cc @davidfowl

@benaadams benaadams force-pushed the context-pooling-base branch from 45f61c7 to 116b599 Compare June 30, 2019 15:01
@analogrelay analogrelay added this to the 3.0.0-preview8 milestone Jul 2, 2019

Protocol = request.Protocol;
Method = request.Method;
ContentType = !string.IsNullOrEmpty(request.ContentType) ? Uri.EscapeUriString(request.ContentType) : string.Empty;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allocations!

@davidfowl
Copy link
Member

I think this change is fine but there's no issue with console and other loggers that store messages in a queue for further processing (at least the ones that we ship). They materialize the message before en-queuing.

@benaadams
Copy link
Member Author

there's no issue with console and other loggers that store messages

If you are happy with the current state, I'll close this as it does make HostingRequestStartingLog a bit bloaty

@neuecc
Copy link

neuecc commented Jun 27, 2024

@davidfowl @benaadams

The logger I'm developing has encountered this issue:
https://github.com/Cysharp/ZLogger

ZLogger adopts an architecture that consistently delays formatting and writes to an IBufferWriter<byte> to avoid allocating strings each time and for performance batching.
As a result, this logging, which captures the HttpContext, is causing problems:
Cysharp/ZLogger#165

Looking at the PR, it seems that incorporating this would solve the issue.
Is there still a possibility of reconsidering and merging this?

Also, I think it's bad to capture the HttpContext.
I were aware that the presence of such a capture could cause problems, but I did not believe that there was anything in the standard that would do such a behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-hosting Includes Hosting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HostingRequestFinishedLog shouldn't capture HttpContext for its values HostingRequestStartingLog shouldn't capture HttpRequest for its values
6 participants