Skip to content
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

Log on reset socket even if the logger is disabled #47620

Closed
1 task done
haludi opened this issue Apr 10, 2023 · 3 comments
Closed
1 task done

Log on reset socket even if the logger is disabled #47620

haludi opened this issue Apr 10, 2023 · 3 comments
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved

Comments

@haludi
Copy link

haludi commented Apr 10, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

If a SocketException exception is thrown while the SocketConnectionListener is accepting a new connection, the logger will still log, even if the logger is disabled

No IsEnabled check in the call position:
https://github.com/dotnet/aspnetcore/blob/release/7.0/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs#L90
https://github.com/dotnet/aspnetcore/blob/release/6.0/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs#L90

And explicitly skip the check on the code generation:
https://github.com/dotnet/aspnetcore/blob/release/7.0/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketsLog.cs#L50
https://github.com/dotnet/aspnetcore/blob/release/6.0/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketsTrace.cs#L70

Expected Behavior

The expected behavior is to call to IsEnabled at some point and not log if a call to it should return false

Steps To Reproduce

Implement Microsoft.Extensions.Logging.ILogger
Implement the Log method to only log without checking if should log (relay only on IsEnabled)
Define the system to use the custom logger.
Start a socket connection and reset it while the server accepts it (I didn't find how to do that).the

Exceptions (if any)

No response

.NET Version

6.0, 7.0

Anything else?

Some related PRs and issues:

#31562 (comment)

#31332
dotnet/runtime#51927
dotnet/runtime#50334

@haludi haludi changed the title Log on reset soket even if the logger is disebleed Log on reset socket even if the logger is disebleed Apr 10, 2023
@haludi haludi changed the title Log on reset socket even if the logger is disebleed Log on reset socket even if the logger is disebled Apr 10, 2023
@haludi haludi changed the title Log on reset socket even if the logger is disebled Log on reset socket even if the logger is disabled Apr 10, 2023
@Tratcher
Copy link
Member

The IsEnabled check is only an optimization recommended when generating the contents of the log is expensive. The ILogger itself is supposed to check if it's enabled before processing messages. The linked logs don't seem expensive to generate?

@haludi
Copy link
Author

haludi commented Apr 10, 2023

The IsEnabled check is only an optimization recommended when generating the contents of the log is expensive. The ILogger itself is supposed to check if it's enabled before processing messages. The linked logs don't seem expensive to generate?

Ok, if this behavior is by design, then the issue can be closed.

@Tratcher Tratcher added the ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. label Apr 11, 2023
@ghost ghost added the Status: Resolved label Apr 11, 2023
@ghost
Copy link

ghost commented Apr 12, 2023

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

@ghost ghost closed this as completed Apr 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 12, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved
Projects
None yet
Development

No branches or pull requests

3 participants