-
Notifications
You must be signed in to change notification settings - Fork 2k
[nightly] Update dependencies from dotnet/installer #4448
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
[nightly] Update dependencies from dotnet/installer #4448
Conversation
60b54c2
to
8776be4
Compare
8776be4
to
a7998b0
Compare
a7998b0
to
1cb744e
Compare
@jander-msft - A Monitor test is failing when running with the Preview 2 build. Can you investigate? |
Sure thing, I'll investigate this today. |
Something changed in ASP.NET Core that is now failing to start .NET Monitor regarding the lack of HTTPS certificate. .NET Monitor was written in such a way that Kestrel should not be automatically attempting to bind all of the endpoints and the tool itself does it manually so that the lack of HTTPS certificate doesn't bring down the process because the metrics endpoints should still be able to bind.
cc @wiktork |
This PR looks related: dotnet/aspnetcore#46296. cc @amcasey |
cc @halter73 who may be able to psychically debug faster than I can investigate. |
@mthalman is there a simple way to repro that callstack locally? |
@jander-msft - Can you provide your simplest repro, ideally outside of Docker? |
Aside: Looks like we might need to handle dev17 here. |
I've managed to build |
Think I got it. If anyone's following along: Add a reference to <ItemGroup Condition="'$(ToolTargetFrameworks)' == 'net8.0'">
<Reference Include="e:\aspnetcore\artifacts\bin\Microsoft.AspNetCore\Release\net8.0\Microsoft.AspNetCore.Server.Kestrel.Core.dll" />
</ItemGroup> Build
Run
Edit: I was able to repro the issue, but I turned out to be using the nightly build of kestrel and not my local binary. I won't look for a better way since we've already decided to revert. |
Alright. I went looking for how the lack of a certificate was ignored on a per-endpoint basis, and found this scary Kestrel configuration callback 😨: And where the exception is eagerly expected from the call to Then I found an old issue I filed on behalf of @wiktork when he was working on dotnet-monitor (dotnet/aspnetcore#28120). It asks for a more first-class way to support this scenario. The suggestion at the time was preloading config, but really we just need a way to skip HTTPS binding if there's no certifcate. Our best bet might be to preload config like the issue suggests. We're going to do it eventually anyway. Then maybe we go back to throwing from I think we should revert dotnet/aspnetcore#46296 for now while we figure out if there's a way to allow dotnet-monitor to preserve its existing behavior while fixing dotnet/aspnetcore#45801. |
The scary callback is due to two aspects that we were working arround:
|
This reverts commit 303115a. Changing the order in which various certificate loading operations are performed broke dotnet-monitor. dotnet/dotnet-docker#4448 (comment)
Revert is here: dotnet/aspnetcore#46868 |
…6868) This reverts commit 303115a. Changing the order in which various certificate loading operations are performed broke dotnet-monitor. dotnet/dotnet-docker#4448 (comment)
Revert is merged. |
Discarded |
1cb744e
to
7ef757e
Compare
No description provided.