-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Check for sentinel value when setting HTTP/3 error code #57934
base: main
Are you sure you want to change the base?
Conversation
If no error code has been set, `IProtocolErrorFeature.Error` will be `-1`. If we pass that through verbatim, it will be caught by validation in the setter (ironically, of the same property on the same feature object), resulting in an exception and a Critical (but apparently benign) log message. Fixes dotnet#57933
This needs a regression test, but I figured I'd run the approach by people before figuring out how to reliably simulate connection timeouts. |
Seems good to me. I see that we're calling Do we plan to backport this? |
If the critical message is indeed benign, this seems fine to fix in 9.0 servicing to me. |
AFAICT, it's benign. It only happens as the connection's being aborted and other connections should be unaffected. Of course, as I type this, it occurs to me I should confirm that this doesn't prevent the connection from being torn down... |
I was going to propose reverting #55282 as a safer fix (since it was made to prevent a benign assert in S.N.Q), but I see from the description of that PR that S.N.Q. probably made checks stricter once we stopped passing them bad data, so a forward fix may be our only option. |
Hmm, it looks like we were expecting to hit this handler when the connection timed out: aspnetcore/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.cs Line 162 in d87133f
|
I don't think this is the same thing as a keep alive timeout. aspnetcore/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.cs Lines 166 to 167 in d87133f
What are we actually seeing for a keep alive timeout? Given that we appear to cancel the token passed into aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs Lines 123 to 124 in d05f358
@JamesNK Does that line up with your expectations? |
I was in the debug-only catch-all block and the code was Edit: I still see connection idle - I put my breakpoint in the wrong place. |
Anecdata: var client = new HttpClient()
{
BaseAddress = new Uri("https://localhost:5001"),
DefaultRequestVersion = new Version(3, 0),
DefaultVersionPolicy = HttpVersionPolicy.RequestVersionExact,
};
try
{
var response = await client.GetAsync("/");
response.EnsureSuccessStatusCode();
var data = await response.Content.ReadAsStringAsync();
Console.WriteLine(data);
}
catch (HttpRequestException e)
{
Console.WriteLine($"Request error: {e.Message}");
}
//Thread.Sleep(TimeSpan.FromSeconds(30)); // ConnectionIdle with Sleep, ConnectionTimeout without
client.Dispose(); I'm interpreting that as |
Interesting. I wouldn't have expected aspnetcore/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionListener.cs Line 89 in d05f358
|
I'm having trouble figuring out whether the unexpected |
FWIW, S.N.Q is getting it via native callback, presumably from msquic: > System.Net.Quic.QuicConnection.HandleEventShutdownInitiatedByTransport(ref Microsoft.Quic.QUIC_CONNECTION_EVENT._Anonymous_e__Union._SHUTDOWN_INITIATED_BY_TRANSPORT_e__Struct) Line 659 C#
System.Net.Quic.QuicConnection.HandleConnectionEvent(ref Microsoft.Quic.QUIC_CONNECTION_EVENT) Line 761 C#
System.Net.Quic.QuicConnection.NativeCallback(Microsoft.Quic.QUIC_HANDLE*, void*, Microsoft.Quic.QUIC_CONNECTION_EVENT*) Line 796 C# |
If the exception occurs, these lines won't be executed KestrelMetrics.AddConnectionEndReason(MetricsContext, reason);
if (TryStopAcceptingStreams())
{
SendGoAwayAsync(GetCurrentGoAwayStreamId()).Preserve();
}
_multiplexedContext.Abort(ex); Losing the metric is a bummer, but not shipblocking (IMO). Edit: the catch block calls Then, in // Wait for active requests to complete.
while (_activeRequestCount > 0)
{
await _streamCompletionAwaitable;
}
TimeoutControl.CancelTimeout(); I'm guessing, but haven't verified, that the stream count has to be zero for the connection to time out? Edit: I'm not convinced this is the case I'm guessing, but haven't verified, that not cancelling the timeout is suboptimal but harmless. Edit: I'm still reasonably sure of this, since the control is per-connection. I guess it's possible a timeout could fire, but the connection should already have been aborted, so it shouldn't do anything worse than log unnecessarily? |
options.Limits.KeepAliveTimeout = TimeSpan.FromSeconds(5); Edit: Stephen pointed out that I was once again being bitten by the fact that we ignore timeouts when a debugger is attached. |
Co-authored-by: James Newton-King <[email protected]>
FYI @ManickaP that there are late changes in this area. I don't expect S.N.Q to have to react, but any feedback would be welcome. Any thoughts on ConnectionIdle would also be appreciated. |
It seems unlikely that this change to |
If no error code has been set,
IProtocolErrorFeature.Error
will be-1
. If we pass that through verbatim, it will be caught by validation in the setter (ironically, of the same property on the same feature object), resulting in an exception and a Critical (but apparently benign) log message.Fixes #57933