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
Enable nullable in Kestrel.Core #28050
Conversation
@@ -418,7 +421,7 @@ internal void ExportCertificate(X509Certificate2 certificate, string path, bool | |||
bytes = certificate.Export(X509ContentType.Pkcs12, password); | |||
break; | |||
case CertificateKeyExportFormat.Pem: | |||
key = certificate.GetRSAPrivateKey(); | |||
key = certificate.GetRSAPrivateKey()!; // TODO - what if PEM doesn't have a private key? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any tests with a PEM file that doesn't have a private key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A private key is required to function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. At the moment I think that no private key will NRE instead of giving a useful error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably throw a useful error and add a test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -13,6 +13,6 @@ public interface IHostContextContainer<TContext> where TContext : notnull | |||
/// <summary> | |||
/// Represents the <typeparamref name="TContext"/> of the host. | |||
/// </summary> | |||
TContext HostContext { get; set; } | |||
TContext? HostContext { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a little weird that TContext has generic constraint of notnull, but the only place it is used is nullable.
Is notnull a way of saying "this type can be anything but Nullable<Struct>
"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the way it is because if flows from IHttpApplication<TContext>
which has the same constraint. You can use your own implementation of that if you call IServer.StartAsync directly, and we probably don't want to have to reason about IHttpApplication<HttpContext?>
and the like.
src/Servers/Kestrel/Core/src/Features/IHttpMinRequestBodyDataRateFeature.cs
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Features/IHttpMinResponseDataRateFeature.cs
Show resolved
Hide resolved
@@ -1171,7 +1173,7 @@ private HttpResponseHeaders CreateResponseHeaders(bool appCompleted) | |||
{ | |||
if ((option.Protocols & HttpProtocols.Http3) == HttpProtocols.Http3) | |||
{ | |||
responseHeaders.HeaderAltSvc = $"h3-25=\":{option.IPEndPoint.Port}\"; ma=84600"; | |||
responseHeaders.HeaderAltSvc = $"h3-25=\":{option.IPEndPoint!.Port}\"; ma=84600"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know that option.IPEndPoint isn't null here @jkotalik? Now that IMultiplexedConnectionListenerFactory is experimentally public, I figure it's theoretically possible. I might just add a null check on option.IPEndPoint in the above if condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to leave this as it. This is experimental, and at the moment I think an exception to say that something is wrong is preferable to silently doing the wrong thing.
src/Servers/Kestrel/Core/src/Internal/Infrastructure/TransportManager.cs
Outdated
Show resolved
Hide resolved
reloadToken = Options.ConfigurationLoader.Configuration.GetReloadToken(); | ||
var (endpointsToStop, endpointsToStart) = Options.ConfigurationLoader.Reload(); | ||
|
||
Trace.LogDebug("Config reload token fired. Checking for changes..."); | ||
|
||
if (endpointsToStop.Count > 0) | ||
{ | ||
var urlsToStop = endpointsToStop.Select(lo => lo.EndpointConfig.Url ?? "<unknown>"); | ||
var urlsToStop = endpointsToStop.Select(lo => lo.EndpointConfig?.Url ?? "<unknown>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: ListenOptions in endpointsToStop must have a non-null EndpointConfig since it comes from the KestrelConfigurationLoader.
var urlsToStop = endpointsToStop.Select(lo => lo.EndpointConfig?.Url ?? "<unknown>"); | |
var urlsToStop = endpointsToStop.Select(lo => lo.EndpointConfig!.Url ?? "<unknown>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Url can't be null because ConfigurationReader throws an error when it is null or empty.
Can all the "<unknown>"
be removed from this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can all the "" be removed from this file?
I believe so. If EndpointConfig isn't null, Url shouldn't be null either.
src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs
Outdated
Show resolved
Hide resolved
e53f6d2
to
ac0f4f7
Compare
298f2ea
to
7bf5857
Compare
Addresses #5680
Fixes #28195