diff --git a/eng/scripts/CodeCheck.ps1 b/eng/scripts/CodeCheck.ps1 index c3e02ff55716..73691561e618 100644 --- a/eng/scripts/CodeCheck.ps1 +++ b/eng/scripts/CodeCheck.ps1 @@ -213,7 +213,7 @@ try { } } # Check for changes in Unshipped in servicing branches - if ($targetBranch -like 'release*' -and $targetBranch -notlike '*preview*' -and $file -like '*PublicAPI.Unshipped.txt') { + if ($targetBranch -like 'release*' -and $targetBranch -notlike '*preview*' -and $targetBranch -notlike '*rc*' -and $file -like '*PublicAPI.Unshipped.txt') { $changedAPIBaselines.Add($file) } } diff --git a/src/Components/Components.slnf b/src/Components/Components.slnf index 14878520e4d5..7bd92c90828f 100644 --- a/src/Components/Components.slnf +++ b/src/Components/Components.slnf @@ -46,23 +46,27 @@ "src\\Components\\test\\E2ETestMigration\\Microsoft.AspNetCore.Components.Migration.E2ETests.csproj", "src\\Components\\test\\E2ETest\\Microsoft.AspNetCore.Components.E2ETests.csproj", "src\\Components\\test\\testassets\\BasicTestApp\\BasicTestApp.csproj", + "src\\Components\\test\\testassets\\ComponentsApp.App\\ComponentsApp.App.csproj", "src\\Components\\test\\testassets\\ComponentsApp.Server\\ComponentsApp.Server.csproj", "src\\Components\\test\\testassets\\GlobalizationWasmApp\\GlobalizationWasmApp.csproj", + "src\\Components\\test\\testassets\\LazyTestContentPackage\\LazyTestContentPackage.csproj", "src\\Components\\test\\testassets\\TestContentPackage\\TestContentPackage.csproj", "src\\Components\\test\\testassets\\TestServer\\Components.TestServer.csproj", "src\\DataProtection\\Abstractions\\src\\Microsoft.AspNetCore.DataProtection.Abstractions.csproj", "src\\DataProtection\\Cryptography.Internal\\src\\Microsoft.AspNetCore.Cryptography.Internal.csproj", "src\\DataProtection\\Cryptography.KeyDerivation\\src\\Microsoft.AspNetCore.Cryptography.KeyDerivation.csproj", "src\\DataProtection\\DataProtection\\src\\Microsoft.AspNetCore.DataProtection.csproj", + "src\\DataProtection\\Extensions\\src\\Microsoft.AspNetCore.DataProtection.Extensions.csproj", "src\\DefaultBuilder\\src\\Microsoft.AspNetCore.csproj", + "src\\Extensions\\Features\\src\\Microsoft.Extensions.Features.csproj", "src\\Features\\JsonPatch\\src\\Microsoft.AspNetCore.JsonPatch.csproj", + "src\\FileProviders\\Embedded\\src\\Microsoft.Extensions.FileProviders.Embedded.csproj", "src\\Hosting\\Abstractions\\src\\Microsoft.AspNetCore.Hosting.Abstractions.csproj", "src\\Hosting\\Hosting\\src\\Microsoft.AspNetCore.Hosting.csproj", "src\\Hosting\\Server.Abstractions\\src\\Microsoft.AspNetCore.Hosting.Server.Abstractions.csproj", "src\\Html.Abstractions\\src\\Microsoft.AspNetCore.Html.Abstractions.csproj", "src\\Http\\Authentication.Abstractions\\src\\Microsoft.AspNetCore.Authentication.Abstractions.csproj", "src\\Http\\Authentication.Core\\src\\Microsoft.AspNetCore.Authentication.Core.csproj", - "src\\Extensions\\Features\\src\\Microsoft.Extensions.Features.csproj", "src\\Http\\Headers\\src\\Microsoft.Net.Http.Headers.csproj", "src\\Http\\Http.Abstractions\\src\\Microsoft.AspNetCore.Http.Abstractions.csproj", "src\\Http\\Http.Extensions\\src\\Microsoft.AspNetCore.Http.Extensions.csproj", @@ -79,6 +83,8 @@ "src\\Identity\\Extensions.Stores\\src\\Microsoft.Extensions.Identity.Stores.csproj", "src\\Identity\\UI\\src\\Microsoft.AspNetCore.Identity.UI.csproj", "src\\JSInterop\\Microsoft.JSInterop\\src\\Microsoft.JSInterop.csproj", + "src\\Localization\\Abstractions\\src\\Microsoft.Extensions.Localization.Abstractions.csproj", + "src\\Localization\\Localization\\src\\Microsoft.Extensions.Localization.csproj", "src\\Middleware\\CORS\\src\\Microsoft.AspNetCore.Cors.csproj", "src\\Middleware\\Diagnostics.Abstractions\\src\\Microsoft.AspNetCore.Diagnostics.Abstractions.csproj", "src\\Middleware\\Diagnostics\\src\\Microsoft.AspNetCore.Diagnostics.csproj", @@ -96,6 +102,7 @@ "src\\Mvc\\Mvc.Core\\src\\Microsoft.AspNetCore.Mvc.Core.csproj", "src\\Mvc\\Mvc.Cors\\src\\Microsoft.AspNetCore.Mvc.Cors.csproj", "src\\Mvc\\Mvc.DataAnnotations\\src\\Microsoft.AspNetCore.Mvc.DataAnnotations.csproj", + "src\\Mvc\\Mvc.Formatters.Json\\src\\Microsoft.AspNetCore.Mvc.Formatters.Json.csproj", "src\\Mvc\\Mvc.Localization\\src\\Microsoft.AspNetCore.Mvc.Localization.csproj", "src\\Mvc\\Mvc.NewtonsoftJson\\src\\Microsoft.AspNetCore.Mvc.NewtonsoftJson.csproj", "src\\Mvc\\Mvc.RazorPages\\src\\Microsoft.AspNetCore.Mvc.RazorPages.csproj", @@ -103,6 +110,7 @@ "src\\Mvc\\Mvc.TagHelpers\\src\\Microsoft.AspNetCore.Mvc.TagHelpers.csproj", "src\\Mvc\\Mvc.ViewFeatures\\src\\Microsoft.AspNetCore.Mvc.ViewFeatures.csproj", "src\\Mvc\\Mvc\\src\\Microsoft.AspNetCore.Mvc.csproj", + "src\\ObjectPool\\src\\Microsoft.Extensions.ObjectPool.csproj", "src\\Razor\\Razor.Runtime\\src\\Microsoft.AspNetCore.Razor.Runtime.csproj", "src\\Razor\\Razor\\src\\Microsoft.AspNetCore.Razor.csproj", "src\\Security\\Authentication\\Cookies\\src\\Microsoft.AspNetCore.Authentication.Cookies.csproj", @@ -128,7 +136,8 @@ "src\\SignalR\\common\\SignalR.Common\\src\\Microsoft.AspNetCore.SignalR.Common.csproj", "src\\SignalR\\server\\Core\\src\\Microsoft.AspNetCore.SignalR.Core.csproj", "src\\SignalR\\server\\SignalR\\src\\Microsoft.AspNetCore.SignalR.csproj", - "src\\Testing\\src\\Microsoft.AspNetCore.Testing.csproj" + "src\\Testing\\src\\Microsoft.AspNetCore.Testing.csproj", + "src\\WebEncoders\\src\\Microsoft.Extensions.WebEncoders.csproj" ] } -} +} \ No newline at end of file diff --git a/src/Components/Web/src/Forms/InputExtensions.cs b/src/Components/Web/src/Forms/InputExtensions.cs index f0065f0611ff..32f8b46c838d 100644 --- a/src/Components/Web/src/Forms/InputExtensions.cs +++ b/src/Components/Web/src/Forms/InputExtensions.cs @@ -16,23 +16,61 @@ internal static class InputExtensions { try { - if (BindConverter.TryConvertTo(value, CultureInfo.CurrentCulture, out var parsedValue)) + // We special-case bool values because BindConverter reserves bool conversion for conditional attributes. + if (typeof(TValue) == typeof(bool)) + { + if (TryConvertToBool(value, out result)) + { + validationErrorMessage = null; + return true; + } + } + else if (typeof(TValue) == typeof(bool?)) + { + if (TryConvertToNullableBool(value, out result)) + { + validationErrorMessage = null; + return true; + } + } + else if (BindConverter.TryConvertTo(value, CultureInfo.CurrentCulture, out var parsedValue)) { result = parsedValue; validationErrorMessage = null; return true; } - else - { - result = default; - validationErrorMessage = $"The {input.DisplayName ?? input.FieldIdentifier.FieldName} field is not valid."; - return false; - } + + result = default; + validationErrorMessage = $"The {input.DisplayName ?? input.FieldIdentifier.FieldName} field is not valid."; + return false; } catch (InvalidOperationException ex) { throw new InvalidOperationException($"{input.GetType()} does not support the type '{typeof(TValue)}'.", ex); } } + + private static bool TryConvertToBool(string? value, out TValue result) + { + if (bool.TryParse(value, out var @bool)) + { + result = (TValue)(object)@bool; + return true; + } + + result = default!; + return false; + } + + private static bool TryConvertToNullableBool(string? value, out TValue result) + { + if (string.IsNullOrEmpty(value)) + { + result = default!; + return true; + } + + return TryConvertToBool(value, out result); + } } } diff --git a/src/Components/Web/src/Forms/InputSelect.cs b/src/Components/Web/src/Forms/InputSelect.cs index 477c8d69326b..cc4ec970b571 100644 --- a/src/Components/Web/src/Forms/InputSelect.cs +++ b/src/Components/Web/src/Forms/InputSelect.cs @@ -64,6 +64,22 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) protected override bool TryParseValueFromString(string? value, [MaybeNullWhen(false)] out TValue result, [NotNullWhen(false)] out string? validationErrorMessage) => this.TryParseSelectableValueFromString(value, out result, out validationErrorMessage); + /// + protected override string? FormatValueAsString(TValue? value) + { + // We special-case bool values because BindConverter reserves bool conversion for conditional attributes. + if (typeof(TValue) == typeof(bool)) + { + return (bool)(object)value! ? "true" : "false"; + } + else if (typeof(TValue) == typeof(bool?)) + { + return value is not null && (bool)(object)value ? "true" : "false"; + } + + return base.FormatValueAsString(value); + } + private void SetCurrentValueAsStringArray(string?[]? value) { CurrentValue = BindConverter.TryConvertTo(value, CultureInfo.CurrentCulture, out var result) diff --git a/src/Components/Web/src/PublicAPI.Unshipped.txt b/src/Components/Web/src/PublicAPI.Unshipped.txt index 3463104ba853..5f28157c5bb1 100644 --- a/src/Components/Web/src/PublicAPI.Unshipped.txt +++ b/src/Components/Web/src/PublicAPI.Unshipped.txt @@ -60,6 +60,7 @@ Microsoft.AspNetCore.Components.Web.PageTitle.ChildContent.set -> void Microsoft.AspNetCore.Components.Web.PageTitle.PageTitle() -> void override Microsoft.AspNetCore.Components.Forms.InputDate.OnParametersSet() -> void abstract Microsoft.AspNetCore.Components.RenderTree.WebRenderer.AttachRootComponentToBrowser(int componentId, string! domElementSelector) -> void +override Microsoft.AspNetCore.Components.Forms.InputSelect.FormatValueAsString(TValue? value) -> string? override Microsoft.AspNetCore.Components.RenderTree.WebRenderer.Dispose(bool disposing) -> void override Microsoft.AspNetCore.Components.Routing.FocusOnNavigate.OnAfterRenderAsync(bool firstRender) -> System.Threading.Tasks.Task! override Microsoft.AspNetCore.Components.Routing.FocusOnNavigate.OnParametersSet() -> void diff --git a/src/Components/test/E2ETest/Tests/FormsTest.cs b/src/Components/test/E2ETest/Tests/FormsTest.cs index d1bbdf36f910..c95813707567 100644 --- a/src/Components/test/E2ETest/Tests/FormsTest.cs +++ b/src/Components/test/E2ETest/Tests/FormsTest.cs @@ -355,6 +355,34 @@ public void InputSelectInteractsWithEditContext() Browser.Equal(new[] { "The TicketClass field is not valid." }, messagesAccessor); } + [Fact] + public void InputSelectInteractsWithEditContext_BoolValues() + { + var appElement = MountTypicalValidationComponent(); + var ticketClassInput = new SelectElement(appElement.FindElement(By.ClassName("select-bool-values")).FindElement(By.TagName("select"))); + var select = ticketClassInput.WrappedElement; + var messagesAccessor = CreateValidationMessagesAccessor(appElement); + + // Invalidates on edit + Browser.Equal("valid", () => select.GetAttribute("class")); + ticketClassInput.SelectByText("true"); + Browser.Equal("modified invalid", () => select.GetAttribute("class")); + Browser.Equal(new[] { "77 + 33 = 100 is a false statement, unfortunately." }, messagesAccessor); + + // Nullable conversion can fail + ticketClassInput.SelectByText("(select)"); + Browser.Equal("modified invalid", () => select.GetAttribute("class")); + Browser.Equal(new[] + { + "77 + 33 = 100 is a false statement, unfortunately.", + "The IsSelectMathStatementTrue field is not valid." + }, messagesAccessor); + + // Can become valid + ticketClassInput.SelectByText("false"); + Browser.Equal("modified valid", () => select.GetAttribute("class")); + } + [Fact] public void InputSelectInteractsWithEditContext_MultipleAttribute() { @@ -521,6 +549,42 @@ public void InputRadioGroupsWithNamesNestedInteractWithEditContext() IReadOnlyCollection FindColorInputs() => group.FindElements(By.Name("color")); } + [Fact] + public void InputRadioGroupWithBoolValuesInteractsWithEditContext() + { + var appElement = MountTypicalValidationComponent(); + var messagesAccessor = CreateValidationMessagesAccessor(appElement); + + // Validate selected inputs + Browser.False(() => FindTrueInput().Selected); + Browser.True(() => FindFalseInput().Selected); + + // Validates on edit + Browser.Equal("valid", () => FindTrueInput().GetAttribute("class")); + Browser.Equal("valid", () => FindFalseInput().GetAttribute("class")); + + FindTrueInput().Click(); + + Browser.Equal("modified valid", () => FindTrueInput().GetAttribute("class")); + Browser.Equal("modified valid", () => FindFalseInput().GetAttribute("class")); + + // Can become invalid + FindFalseInput().Click(); + + Browser.Equal("modified invalid", () => FindTrueInput().GetAttribute("class")); + Browser.Equal("modified invalid", () => FindFalseInput().GetAttribute("class")); + Browser.Equal(new[] { "7 * 3 = 21 is a true statement." }, messagesAccessor); + + IReadOnlyCollection FindInputs() + => appElement.FindElement(By.ClassName("radio-group-bool-values")).FindElements(By.TagName("input")); + + IWebElement FindTrueInput() + => FindInputs().First(i => string.Equals("True", i.GetAttribute("value"))); + + IWebElement FindFalseInput() + => FindInputs().First(i => string.Equals("False", i.GetAttribute("value"))); + } + [Fact] public void CanWireUpINotifyPropertyChangedToEditContext() { diff --git a/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor b/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor index 458759f9ab62..3fb9217a5fbd 100644 --- a/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor +++ b/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor @@ -70,6 +70,14 @@ @string.Join(", ", person.HostileStrings)

+

+ T/F: 77 + 33 = 100
+ + + + + +

Airline: @@ -96,6 +104,13 @@

+

+ T/F: 7 * 3 = 21
+ + true
+ false
+ +

Socks color:

@@ -188,6 +203,12 @@ [Required, EnumDataType(typeof(Country))] public Country? Country { get; set; } = null; + [Required, Range(typeof(bool), "false", "false", ErrorMessage = "77 + 33 = 100 is a false statement, unfortunately.")] + public bool? IsSelectMathStatementTrue { get; set; } = null; + + [Required, Range(typeof(bool), "true", "true", ErrorMessage = "7 * 3 = 21 is a true statement.")] + public bool IsRadioMathStatementTrue { get; set; } = false; + [Required, StringLength(10), CustomValidationClassName(Valid = "valid-socks", Invalid = "invalid-socks")] public string SocksColor { get; set; } diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 94d04695dc99..a735b9abee20 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -627,8 +627,7 @@ private static Expression GetValueFromProperty(Expression sourceExpression, stri private static Expression BindParameterFromService(ParameterInfo parameter) { - var nullability = NullabilityContext.Create(parameter); - var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable; + var isOptional = IsOptionalParameter(parameter); return isOptional ? Expression.Call(GetServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr) @@ -637,8 +636,7 @@ private static Expression BindParameterFromService(ParameterInfo parameter) private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, FactoryContext factoryContext) { - var nullability = NullabilityContext.Create(parameter); - var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable; + var isOptional = IsOptionalParameter(parameter); var argument = Expression.Variable(parameter.ParameterType, $"{parameter.Name}_local"); @@ -671,7 +669,8 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres } // Allow nullable parameters that don't have a default value - if (nullability.ReadState == NullabilityState.Nullable && !parameter.HasDefaultValue) + var nullability = NullabilityContext.Create(parameter); + if (nullability.ReadState != NullabilityState.NotNull && !parameter.HasDefaultValue) { return valueExpression; } @@ -817,7 +816,7 @@ private static Expression BindParameterFromBindAsync(ParameterInfo parameter, Fa { // We reference the boundValues array by parameter index here var nullability = NullabilityContext.Create(parameter); - var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable; + var isOptional = IsOptionalParameter(parameter); // Get the BindAsync method var body = TryParseMethodCache.FindBindAsyncMethod(parameter.ParameterType)!; @@ -862,8 +861,7 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al } } - var nullability = NullabilityContext.Create(parameter); - var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable; + var isOptional = IsOptionalParameter(parameter); factoryContext.JsonRequestBodyType = parameter.ParameterType; factoryContext.AllowEmptyRequestBody = allowEmpty || isOptional; @@ -903,6 +901,19 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al return Expression.Convert(BodyValueExpr, parameter.ParameterType); } + private static bool IsOptionalParameter(ParameterInfo parameter) + { + // - Parameters representing value or reference types with a default value + // under any nullability context are treated as optional. + // - Value type parameters without a default value in an oblivious + // nullability context are required. + // - Reference type parameters without a default value in an oblivious + // nullability context are optional. + var nullability = NullabilityContext.Create(parameter); + return parameter.HasDefaultValue + || nullability.ReadState != NullabilityState.NotNull; + } + private static MethodInfo GetMethodInfo(Expression expr) { var mc = (MethodCallExpression)expr.Body; diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 25b5ffe6750e..a6236e37c802 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -2137,6 +2137,35 @@ public async Task CanSetParseableStringParamAsOptionalWithNullabilityDisability( Assert.Equal(expectedResponse, decodedResponseBody); } + [Theory] + [InlineData(true, "Age: 42")] + [InlineData(false, "Age: ")] + public async Task TreatsUnknownNullabilityAsOptionalForReferenceType(bool provideValue, string expectedResponse) + { + string optionalQueryParam(string age) => $"Age: {age}"; + + var httpContext = new DefaultHttpContext(); + var responseBodyStream = new MemoryStream(); + httpContext.Response.Body = responseBodyStream; + + if (provideValue) + { + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["age"] = "42" + }); + } + + var requestDelegate = RequestDelegateFactory.Create(optionalQueryParam); + + await requestDelegate(httpContext); + + Assert.Equal(200, httpContext.Response.StatusCode); + Assert.False(httpContext.RequestAborted.IsCancellationRequested); + var decodedResponseBody = Encoding.UTF8.GetString(responseBodyStream.ToArray()); + Assert.Equal(expectedResponse, decodedResponseBody); + } + #nullable enable private class Todo : ITodo diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index 815d1b269259..f64d5b0a3b05 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -143,7 +143,7 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string // Determine the "requiredness" based on nullability, default value or if allowEmpty is set var nullability = NullabilityContext.Create(parameter); - var isOptional = parameter.HasDefaultValue || nullability.ReadState == NullabilityState.Nullable || allowEmpty; + var isOptional = parameter.HasDefaultValue || nullability.ReadState != NullabilityState.NotNull || allowEmpty; return new ApiParameterDescription { diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs index 063199b39513..707a80f8c8e8 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs @@ -340,7 +340,7 @@ public void AddsMultipleParameters() Assert.Equal(typeof(InferredJsonClass), fromBodyParam.Type); Assert.Equal(typeof(InferredJsonClass), fromBodyParam.ModelMetadata.ModelType); Assert.Equal(BindingSource.Body, fromBodyParam.Source); - Assert.True(fromBodyParam.IsRequired); + Assert.False(fromBodyParam.IsRequired); // Reference type in oblivious nullability context } [Fact] @@ -413,6 +413,27 @@ public void AddsMetadataFromRouteEndpoint() Assert.True(apiExplorerSettings.IgnoreApi); } + [Fact] + public void TestParameterIsRequiredForObliviousNullabilityContext() + { + // In an oblivious nullability context, reference type parameters without + // annotations are optional. Value type parameters are always required. + var apiDescription = GetApiDescription((string foo, int bar) => { }); + Assert.Equal(2, apiDescription.ParameterDescriptions.Count); + + var fooParam = apiDescription.ParameterDescriptions[0]; + Assert.Equal(typeof(string), fooParam.Type); + Assert.Equal(typeof(string), fooParam.ModelMetadata.ModelType); + Assert.Equal(BindingSource.Query, fooParam.Source); + Assert.False(fooParam.IsRequired); + + var barParam = apiDescription.ParameterDescriptions[1]; + Assert.Equal(typeof(int), barParam.Type); + Assert.Equal(typeof(int), barParam.ModelMetadata.ModelType); + Assert.Equal(BindingSource.Query, barParam.Source); + Assert.True(barParam.IsRequired); + } + [Fact] public void RespectsProducesProblemExtensionMethod() { diff --git a/src/Servers/HttpSys/src/RequestProcessing/RequestStream.Log.cs b/src/Servers/HttpSys/src/RequestProcessing/RequestStream.Log.cs index 2f223507e156..1421a7ec23e1 100644 --- a/src/Servers/HttpSys/src/RequestProcessing/RequestStream.Log.cs +++ b/src/Servers/HttpSys/src/RequestProcessing/RequestStream.Log.cs @@ -11,13 +11,13 @@ internal partial class RequestStream private static class Log { private static readonly Action _errorWhenReadAsync = - LoggerMessage.Define(LogLevel.Error, LoggerEventIds.ErrorWhenReadAsync, "ReadAsync"); + LoggerMessage.Define(LogLevel.Debug, LoggerEventIds.ErrorWhenReadAsync, "ReadAsync"); private static readonly Action _errorWhenReadBegun = - LoggerMessage.Define(LogLevel.Error, LoggerEventIds.ErrorWhenReadBegun, "BeginRead"); + LoggerMessage.Define(LogLevel.Debug, LoggerEventIds.ErrorWhenReadBegun, "BeginRead"); private static readonly Action _errorWhileRead = - LoggerMessage.Define(LogLevel.Error, LoggerEventIds.ErrorWhileRead, "Read"); + LoggerMessage.Define(LogLevel.Debug, LoggerEventIds.ErrorWhileRead, "Read"); public static void ErrorWhenReadAsync(ILogger logger, Exception exception) { diff --git a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs index 51b17f1331f4..9f4b236e729d 100644 --- a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs +++ b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs @@ -187,7 +187,30 @@ private async Task DoReceive() input.Advance(bytesReceived); - var flushTask = input.FlushAsync(); + ValueTask flushTask; + + if (_stream.ReadsCompleted) + { + // If the data returned from ReadAsync is the final chunk on the stream then + // flush data and end pipe together with CompleteAsync. + // + // Getting data and complete together is important for HTTP/3 when parsing headers. + // It is important that it knows that there is no body after the headers. + var completeTask = input.CompleteAsync(ResolveCompleteReceiveException(error)); + if (completeTask.IsCompletedSuccessfully) + { + // Fast path. CompleteAsync completed immediately. + flushTask = ValueTask.FromResult(new FlushResult(isCanceled: false, isCompleted: true)); + } + else + { + flushTask = AwaitCompleteTaskAsync(completeTask); + } + } + else + { + flushTask = input.FlushAsync(); + } var paused = !flushTask.IsCompleted; @@ -240,12 +263,23 @@ private async Task DoReceive() finally { // If Shutdown() has already bee called, assume that was the reason ProcessReceives() exited. - Input.Complete(_shutdownReadReason ?? _shutdownReason ?? error); + Input.Complete(ResolveCompleteReceiveException(error)); FireStreamClosed(); await _waitForConnectionClosedTcs.Task; } + + async static ValueTask AwaitCompleteTaskAsync(ValueTask completeTask) + { + await completeTask; + return new FlushResult(isCanceled: false, isCompleted: true); + } + } + + private Exception? ResolveCompleteReceiveException(Exception? error) + { + return _shutdownReadReason ?? _shutdownReason ?? error; } private void FireStreamClosed() @@ -415,11 +449,15 @@ private void ShutdownWrite(Exception? shutdownReason) public override async ValueTask DisposeAsync() { + // Be conservative about what can be pooled. + // Only pool bidirectional streams whose pipes have completed successfully and haven't been aborted. CanReuse = _stream.CanRead && _stream.CanWrite && _transportPipeReader.IsCompletedSuccessfully && _transportPipeWriter.IsCompletedSuccessfully && !_clientAbort - && !_serverAborted; + && !_serverAborted + && _shutdownReadReason == null + && _shutdownWriteReason == null; _originalTransport.Input.Complete(); _originalTransport.Output.Complete(); diff --git a/src/Servers/Kestrel/Transport.Quic/test/QuicConnectionListenerTests.cs b/src/Servers/Kestrel/Transport.Quic/test/QuicConnectionListenerTests.cs index ee265994c4cc..83ee6cc72080 100644 --- a/src/Servers/Kestrel/Transport.Quic/test/QuicConnectionListenerTests.cs +++ b/src/Servers/Kestrel/Transport.Quic/test/QuicConnectionListenerTests.cs @@ -115,8 +115,7 @@ public async Task ClientCertificate_Required_NotSent_ConnectionAborted() // https://github.com/dotnet/runtime/issues/57246 The accept still completes even though the connection was rejected, but it's already failed. var serverContext = await connectionListener.AcceptAndAddFeatureAsync().DefaultTimeout(); - qex = await Assert.ThrowsAsync(() => serverContext.ConnectAsync().DefaultTimeout()); - Assert.Equal("Failed to open stream to peer. Error Code: INVALID_STATE", qex.Message); + await Assert.ThrowsAsync(() => serverContext.ConnectAsync().DefaultTimeout()); } } } diff --git a/src/Servers/Kestrel/Transport.Quic/test/QuicStreamContextTests.cs b/src/Servers/Kestrel/Transport.Quic/test/QuicStreamContextTests.cs index 6e8dfced9488..c180230bb3d3 100644 --- a/src/Servers/Kestrel/Transport.Quic/test/QuicStreamContextTests.cs +++ b/src/Servers/Kestrel/Transport.Quic/test/QuicStreamContextTests.cs @@ -49,6 +49,52 @@ public async Task BidirectionalStream_ServerReadsDataAndCompletes_GracefullyClos Assert.Contains(TestSink.Writes, m => m.Message.Contains(@"shutting down writes because: ""The QUIC transport's send loop completed gracefully."".")); } + [ConditionalFact] + [MsQuicSupported] + public async Task BidirectionalStream_ReadAborted_NotPooled() + { + // Arrange + await using var connectionListener = await QuicTestHelpers.CreateConnectionListenerFactory(LoggerFactory); + + var options = QuicTestHelpers.CreateClientConnectionOptions(connectionListener.EndPoint); + using var clientConnection = new QuicConnection(QuicImplementationProviders.MsQuic, options); + await clientConnection.ConnectAsync().DefaultTimeout(); + + await using var serverConnection = await connectionListener.AcceptAndAddFeatureAsync().DefaultTimeout(); + + // Act + var clientStream = clientConnection.OpenBidirectionalStream(); + await clientStream.WriteAsync(TestData).DefaultTimeout(); + var serverStream = await serverConnection.AcceptAsync().DefaultTimeout(); + var readResult = await serverStream.Transport.Input.ReadAtLeastAsync(TestData.Length).DefaultTimeout(); + serverStream.Transport.Input.AdvanceTo(readResult.Buffer.End); + + await clientStream.WriteAsync(TestData).DefaultTimeout(); + + // Complete writing. + await serverStream.Transport.Output.CompleteAsync(); + + // Abort read-side of the stream and then complete pipe. + // This simulates what Kestrel does when a request finishes without + // reading the request body to the end. + serverStream.Features.Get().AbortRead((long)Http3ErrorCode.NoError, new ConnectionAbortedException("Test message.")); + await serverStream.Transport.Input.CompleteAsync(); + + var quicStreamContext = Assert.IsType(serverStream); + + // Both send and receive loops have exited. + await quicStreamContext._processingTask.DefaultTimeout(); + Assert.True(quicStreamContext.CanWrite); + Assert.True(quicStreamContext.CanRead); + + await quicStreamContext.DisposeAsync(); + + var quicConnectionContext = Assert.IsType(serverConnection); + + // Assert + Assert.Equal(0, quicConnectionContext.StreamPool.Count); + } + [ConditionalTheory] [MsQuicSupported] [InlineData(1024)] @@ -255,6 +301,38 @@ public async Task ClientToServerUnidirectionalStream_ClientAbort_ServerReceivesA await closedTcs.Task.DefaultTimeout(); } + [ConditionalFact] + [MsQuicSupported] + public async Task ClientToServerUnidirectionalStream_CompleteWrites_PipeProvidesDataAndCompleteTogether() + { + // Arrange + await using var connectionListener = await QuicTestHelpers.CreateConnectionListenerFactory(LoggerFactory); + + var options = QuicTestHelpers.CreateClientConnectionOptions(connectionListener.EndPoint); + using var quicConnection = new QuicConnection(QuicImplementationProviders.MsQuic, options); + await quicConnection.ConnectAsync().DefaultTimeout(); + + await using var serverConnection = await connectionListener.AcceptAndAddFeatureAsync().DefaultTimeout(); + + // Act + await using var clientStream = quicConnection.OpenUnidirectionalStream(); + await clientStream.WriteAsync(TestData).DefaultTimeout(); + + await using var serverStream = await serverConnection.AcceptAsync().DefaultTimeout(); + var readResult = await serverStream.Transport.Input.ReadAtLeastAsync(TestData.Length).DefaultTimeout(); + serverStream.Transport.Input.AdvanceTo(readResult.Buffer.End); + + var readResultTask = serverStream.Transport.Input.ReadAsync(); + + await clientStream.WriteAsync(TestData, endStream: true).DefaultTimeout(); + + // Assert + var completeReadResult = await readResultTask.DefaultTimeout(); + + Assert.Equal(TestData, completeReadResult.Buffer.ToArray()); + Assert.True(completeReadResult.IsCompleted); + } + [ConditionalFact] [MsQuicSupported] public async Task ServerToClientUnidirectionalStream_ServerWritesDataAndCompletes_GracefullyClosed() diff --git a/src/Servers/Kestrel/test/Interop.FunctionalTests/Http3/Http3RequestTests.cs b/src/Servers/Kestrel/test/Interop.FunctionalTests/Http3/Http3RequestTests.cs index 9eef19cdad97..0bc59decd668 100644 --- a/src/Servers/Kestrel/test/Interop.FunctionalTests/Http3/Http3RequestTests.cs +++ b/src/Servers/Kestrel/test/Interop.FunctionalTests/Http3/Http3RequestTests.cs @@ -672,6 +672,69 @@ public async Task GET_ClientCancellationAfterResponseHeaders_RequestAbortRaised( } } + [ConditionalFact] + [MsQuicSupported] + public async Task StreamResponseContent_DelayAndTrailers_ClientSuccess() + { + // Arrange + var builder = CreateHostBuilder(async context => + { + var feature = context.Features.Get(); + + for (var i = 1; i < 200; i++) + { + feature.Trailers.Append($"trailer-{i}", new string('!', i)); + } + + Logger.LogInformation($"Server trailer count: {feature.Trailers.Count}"); + + await context.Request.BodyReader.ReadAtLeastAsync(TestData.Length); + + for (var i = 0; i < 3; i++) + { + await context.Response.BodyWriter.WriteAsync(TestData); + + await Task.Delay(TimeSpan.FromMilliseconds(10)); + } + }); + + using (var host = builder.Build()) + using (var client = Http3Helpers.CreateClient()) + { + await host.StartAsync(); + + // Act + var request = new HttpRequestMessage(HttpMethod.Post, $"https://127.0.0.1:{host.GetPort()}/"); + request.Content = new ByteArrayContent(TestData); + request.Version = HttpVersion.Version30; + request.VersionPolicy = HttpVersionPolicy.RequestVersionExact; + + var response = await client.SendAsync(request, CancellationToken.None); + response.EnsureSuccessStatusCode(); + + var responseStream = await response.Content.ReadAsStreamAsync(); + + await responseStream.ReadUntilEndAsync(); + + Logger.LogInformation($"Client trailer count: {response.TrailingHeaders.Count()}"); + + for (var i = 1; i < 200; i++) + { + try + { + var value = response.TrailingHeaders.GetValues($"trailer-{i}").Single(); + Assert.Equal(new string('!', i), value); + } + catch (Exception ex) + { + throw new Exception($"Error checking trailer {i}", ex); + } + } + + await host.StopAsync(); + } + } + [ConditionalFact] [MsQuicSupported] public async Task GET_MultipleRequests_ConnectionAndTraceIdsUpdated()