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

Don't log a stack trace for AddressInUseException #30154

Merged
merged 5 commits into from
Feb 23, 2021

Conversation

BerserkerDotNet
Copy link
Contributor

@BerserkerDotNet BerserkerDotNet commented Feb 12, 2021

In case of AddressInUseException do not log the exception
AddressInUseException:
image
image

Other exceptions:
image

Addresses #29801

@dnfadmin
Copy link

dnfadmin commented Feb 12, 2021

CLA assistant check
All CLA requirements met.

@davidfowl
Copy link
Member

Wait why are we taking this change? Isn't this in the wrong layer? Removing information from loggers is the wrong place to do this sort of filtering.

@BerserkerDotNet
Copy link
Contributor Author

BerserkerDotNet commented Feb 12, 2021

Wait why are we taking this change? Isn't this in the wrong layer? Removing information from loggers is the wrong place to do this sort of filtering.

Won't argue, it looks weird at best. :) But my understanding is #29801 asks to remove this information from the log without suppressing the exception. (See #29801 (comment)) Maybe I misunderstood the ask.

@shirhatti shirhatti added the community-contribution Indicates that the PR has been added by a community member label Feb 12, 2021
@BrennanConroy
Copy link
Member

Triage: We think we shouldn't log and just let the exception flow up to the user.

@BerserkerDotNet
Copy link
Contributor Author

Triage: We think we shouldn't log and just let the exception flow up to the user.

Ok, I'll make that change.

@BerserkerDotNet
Copy link
Contributor Author

@BrennanConroy, done. Updated PR description as well.

if (ex is not IOException && ex.InnerException is not AddressInUseException)
{
Trace.LogCritical(0, ex, "Unable to start Kestrel.");
}
Copy link
Member

@halter73 halter73 Feb 22, 2021

Choose a reason for hiding this comment

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

@davidfowl Was your suggestion to no longer log any startup exceptions in KestrelServer? It feels wrong to log everything except AddressInUseExceptions.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, don't log anything here. Just rethrow the exception.

@halter73 halter73 merged commit 465a219 into dotnet:main Feb 23, 2021
@halter73
Copy link
Member

Thanks @BerserkerDotNet!

@BerserkerDotNet BerserkerDotNet deleted the special-handle-port-bind branch February 23, 2021 18:56
@maxcherednik
Copy link

@davidfowl I guess we addressed one of the issues mentioned here: #29801

To be specific - log and throw issue.

There was another issue, which is bigger than the one addressed in this pr.

Can we please consider reopening the issue?

@ghost
Copy link

ghost commented Apr 17, 2022

Hi @maxcherednik. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@davidfowl
Copy link
Member

@maxcherednik , open a new issue please with details.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants