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.

Sign up for GitHub

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
Merged

Don't log a stack trace for AddressInUseException #30154

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57ce636
don't log a stack trace for AddressInUseException
BerserkerDotNet Feb 12, 2021
6fccbb0
tests
BerserkerDotNet Feb 12, 2021
0b6669b
Update src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs
BerserkerDotNet Feb 12, 2021
dd75c17
Don't log exception for AddressInUse
BerserkerDotNet Feb 20, 2021
dc58f3e
don't log the error at all
BerserkerDotNet Feb 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.IO.Pipelines;
using System.Linq;
using System.Threading;
Expand Down Expand Up @@ -207,7 +208,12 @@ async Task OnBind(ListenOptions options)
}
catch (Exception ex)
{
Trace.LogCritical(0, ex, "Unable to start Kestrel.");
// Do not log stack trace for known errors https://github.com/dotnet/aspnetcore/issues/29801
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 •
edited
Loading

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.


Dispose();
throw;
}
Expand Down
6 changes: 6 additions & 0 deletions src/Servers/Kestrel/shared/test/TestApplicationErrorLoggerLoggedTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ public class TestApplicationErrorLoggerLoggedTest : LoggedTest

public ConcurrentQueue<LogMessage> LogMessages => TestApplicationErrorLogger.Messages;

public bool ThrowOnCriticalErrors
{
get => TestApplicationErrorLogger.ThrowOnCriticalErrors;
set => TestApplicationErrorLogger.ThrowOnCriticalErrors = value;
}

public bool ThrowOnUngracefulShutdown
{
get => TestApplicationErrorLogger.ThrowOnUngracefulShutdown;
Expand Down
25 changes: 19 additions & 6 deletions src/Servers/Kestrel/test/BindTests/AddressRegistrationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,8 @@ private async Task RegisterDefaultServerAddresses_Success(IEnumerable<string> ad
[Fact]
public async Task ThrowsWhenBindingToIPv4AddressInUse()
{
ThrowOnCriticalErrors = false;

using (var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
{
socket.Bind(new IPEndPoint(IPAddress.Loopback, 0));
Expand All @@ -562,13 +564,17 @@ public async Task ThrowsWhenBindingToIPv4AddressInUse()
.UseKestrel()
.UseUrls($"http://127.0.0.1:{port}")
.Configure(ConfigureEchoAddress);
});
})
.ConfigureServices(AddTestLogging);

using (var host = hostBuilder.Build())
{
var exception = Assert.Throws<IOException>(() => host.Start());
Assert.Equal(CoreStrings.FormatEndpointAlreadyInUse($"http://127.0.0.1:{port}"), exception.Message);

var expectedMessage = CoreStrings.FormatEndpointAlreadyInUse($"http://127.0.0.1:{port}");
Assert.Equal(expectedMessage, exception.Message);
Assert.Equal(0, LogMessages.Count(log => log.LogLevel == LogLevel.Critical &&
log.Exception is null &&
log.Message.EndsWith(expectedMessage, StringComparison.Ordinal)));
await host.StopAsync();
}
}
Expand All @@ -578,7 +584,7 @@ public async Task ThrowsWhenBindingToIPv4AddressInUse()
[IPv6SupportedCondition]
public async Task ThrowsWhenBindingToIPv6AddressInUse()
{
IgnoredCriticalLogExceptions.Add(typeof(IOException));
ThrowOnCriticalErrors = false;
Tratcher marked this conversation as resolved.
Show resolved Hide resolved

using (var socket = new Socket(AddressFamily.InterNetworkV6, SocketType.Stream, ProtocolType.Tcp))
{
Expand All @@ -599,7 +605,11 @@ public async Task ThrowsWhenBindingToIPv6AddressInUse()
using (var host = hostBuilder.Build())
{
var exception = Assert.Throws<IOException>(() => host.Start());
Assert.Equal(CoreStrings.FormatEndpointAlreadyInUse($"http://[::1]:{port}"), exception.Message);
var expectedMessage = CoreStrings.FormatEndpointAlreadyInUse($"http://[::1]:{port}");
Assert.Equal(expectedMessage, exception.Message);
Assert.Equal(0, LogMessages.Count(log => log.LogLevel == LogLevel.Critical &&
log.Exception is null &&
log.Message.EndsWith(expectedMessage, StringComparison.Ordinal)));

await host.StopAsync();
}
Expand Down Expand Up @@ -931,7 +941,7 @@ public async Task EndpointDefaultsConfig_CanSetProtocolForUrlsConfig(string inpu

private void ThrowsWhenBindingLocalhostToAddressInUse(AddressFamily addressFamily)
{
IgnoredCriticalLogExceptions.Add(typeof(IOException));
ThrowOnCriticalErrors = false;

var addressInUseCount = 0;
var wrongMessageCount = 0;
Expand Down Expand Up @@ -990,6 +1000,9 @@ private void ThrowsWhenBindingLocalhostToAddressInUse(AddressFamily addressFamil
}

Assert.Equal(CoreStrings.FormatEndpointAlreadyInUse(thisAddressString), exception.Message);
Assert.Equal(0, LogMessages.Count(log => log.LogLevel == LogLevel.Critical &&
log.Exception is null &&
log.Message.EndsWith(CoreStrings.FormatEndpointAlreadyInUse(thisAddressString), StringComparison.Ordinal)));
break;
}
}
Expand Down