From 9496fb82cc8c2532ed6a2a6453e11f8d7a11f2e2 Mon Sep 17 00:00:00 2001 From: maliming Date: Fri, 16 Oct 2026 12:46:38 +0800 Subject: [PATCH] Fix unit of work middleware edge cases and document its options - Skip SignalR hub requests in `AbpUnitOfWorkMiddleware` - Don't begin a completed or disposed reservation in `TryBeginReserved` --- .../domain-driven-design/unit-of-work.md | 24 ++++++++++ .../AspNetCore/Uow/AbpUnitOfWorkMiddleware.cs | 8 ++++ .../Volo/Abp/Uow/UnitOfWorkManager.cs | 3 +- ...WorkMiddleware_Exception_Rollback_Tests.cs | 16 +++++++ .../Mvc/Uow/UnitOfWorkTestController.cs | 9 ++++ .../SignalR/AbpAspNetCoreSignalRTestModule.cs | 18 +++++++- .../SignalR/AbpSignalRUnitOfWork_Tests.cs | 38 ++++++++++++++++ .../SignalR/SampleHubs/CustomRouteHub.cs | 6 +++ .../Abp/Uow/UnitOfWork_Ambient_Scope_Tests.cs | 45 +++++++++++++++++++ 9 files changed, 165 insertions(+), 2 deletions(-) create mode 100644 framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/AbpSignalRUnitOfWork_Tests.cs create mode 100644 framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/SampleHubs/CustomRouteHub.cs diff --git a/docs/en/framework/architecture/domain-driven-design/unit-of-work.md b/docs/en/framework/architecture/domain-driven-design/unit-of-work.md index 613620ab139..6b6de422a14 100644 --- a/docs/en/framework/architecture/domain-driven-design/unit-of-work.md +++ b/docs/en/framework/architecture/domain-driven-design/unit-of-work.md @@ -361,6 +361,30 @@ app.UseUnitOfWork(); app.UseConfiguredEndpoints(); ```` +By default, the middleware completes the unit of work at the end of the request pipeline. If the response has already started, a failure (like a transaction commit or a unit of work event handler failure) can not change the status code, so the client may still get a success response. + +`UseUnitOfWork` also adds the exception handling middleware. If you don't use `UseUnitOfWork`, add `UseAbpExceptionHandling` to the same place: + +````csharp +app.UseAbpExceptionHandling(); +app.UseConfiguredEndpoints(); +```` + +#### AbpAspNetCoreUnitOfWorkOptions + +`AbpAspNetCoreUnitOfWorkOptions` is used to configure the middleware: + +````csharp +Configure(options => +{ + options.IgnoredUrls.Add("/api/my-streaming-endpoint"); +}); +```` + +* `IgnoredUrls` (`List`): The middleware is disabled for the requests whose path starts with one of these values. It is also disabled for the [SignalR](../../real-time/signalr.md) hub requests. +* `CompleteUnitOfWorkOnResponseStarting` (`bool`, default: `false`): Completes the unit of work just before the response starts, so a failure can return an error status code (with an empty response body). The changes are committed at that point, so a later failure can not roll them back. It is not suitable for streaming responses. +* `CompleteUnitOfWorkOnResponseStartingUrls` (`List`): Completes the unit of work just before the response starts for the requests whose path starts with one of these values, even if `CompleteUnitOfWorkOnResponseStarting` is `false`. The [OpenIddict module](../../../modules/openiddict.md) adds some of its server endpoints, like the token endpoint, to this list. + ## See Also * [Video tutorial Code](https://abp.io/video-courses/essentials/unit-of-work-code) diff --git a/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpUnitOfWorkMiddleware.cs b/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpUnitOfWorkMiddleware.cs index 64c507d2600..bd19bbf3481 100644 --- a/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpUnitOfWorkMiddleware.cs +++ b/framework/src/Volo.Abp.AspNetCore/Volo/Abp/AspNetCore/Uow/AbpUnitOfWorkMiddleware.cs @@ -3,6 +3,7 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Components.Endpoints; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.SignalR; using Microsoft.Extensions.Options; using Volo.Abp.AspNetCore.Middleware; using Volo.Abp.DependencyInjection; @@ -94,6 +95,13 @@ protected async override Task ShouldSkipAsync(HttpContext context, Request return true; } + // A hub connection request lives as long as the connection. A unit of work reserved for it would be + // shared by all the hub method calls of the connection and can not be completed when the client disconnects. + if (context.GetEndpoint()?.Metadata?.GetMetadata() != null) + { + return true; + } + return await base.ShouldSkipAsync(context, next); } } diff --git a/framework/src/Volo.Abp.Uow/Volo/Abp/Uow/UnitOfWorkManager.cs b/framework/src/Volo.Abp.Uow/Volo/Abp/Uow/UnitOfWorkManager.cs index 970f7030caf..3ce638df72b 100644 --- a/framework/src/Volo.Abp.Uow/Volo/Abp/Uow/UnitOfWorkManager.cs +++ b/framework/src/Volo.Abp.Uow/Volo/Abp/Uow/UnitOfWorkManager.cs @@ -71,7 +71,8 @@ public bool TryBeginReserved(string reservationName, AbpUnitOfWorkOptions option var uow = _ambientUnitOfWork.UnitOfWork; //Find reserved unit of work starting from current and going to outers - while (uow != null && !uow.IsReservedFor(reservationName)) + //A reservation that has ended can still be the ambient one for the code that outlives it + while (uow != null && (!uow.IsReservedFor(reservationName) || uow.IsCompleted || uow.IsDisposed)) { uow = uow.Outer; } diff --git a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkMiddleware_Exception_Rollback_Tests.cs b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkMiddleware_Exception_Rollback_Tests.cs index d959db16329..66dabb75cd9 100644 --- a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkMiddleware_Exception_Rollback_Tests.cs +++ b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkMiddleware_Exception_Rollback_Tests.cs @@ -3,7 +3,9 @@ using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Options; using Shouldly; +using Volo.Abp.AspNetCore.Uow; using Volo.Abp.Http; using Volo.Abp.Json; using Volo.Abp.Uow; @@ -40,4 +42,18 @@ public async Task Should_Gracefully_Handle_Exceptions_On_Complete() result.Error.ShouldNotBeNull(); result.Error.Message.ShouldBe(TestUnitOfWorkConfig.ExceptionOnCompleteMessage); } + + [Fact] + public async Task Should_Return_The_Error_When_Completing_Fails_After_An_Object_Result_Without_The_Middleware() + { + const string url = "/api/unitofwork-test/ObjectResultExceptionOnComplete"; + + // The request runs as if the pipeline only had app.UseAbpExceptionHandling(). + ServiceProvider.GetRequiredService>().Value.IgnoredUrls.Add(url); + + var result = await GetResponseAsObjectAsync(url, HttpStatusCode.Forbidden); + + result.Error.ShouldNotBeNull(); + result.Error.Message.ShouldBe(TestUnitOfWorkConfig.ExceptionOnCompleteMessage); + } } diff --git a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestController.cs b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestController.cs index 77cf3f07b43..58227e855a5 100644 --- a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestController.cs +++ b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestController.cs @@ -74,6 +74,15 @@ public void ExceptionOnComplete() _testUnitOfWorkConfig.ThrowExceptionOnComplete = true; } + [HttpGet] + [Route("ObjectResultExceptionOnComplete")] + public object ObjectResultExceptionOnComplete() + { + _testUnitOfWorkConfig.ThrowExceptionOnComplete = true; + + return new { Value = 42 }; + } + [HttpGet] [Route("CommitBeforeResponseFlush")] public async Task CommitBeforeResponseFlush() diff --git a/framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/AbpAspNetCoreSignalRTestModule.cs b/framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/AbpAspNetCoreSignalRTestModule.cs index f0c056c7843..9fdd54e3e8c 100644 --- a/framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/AbpAspNetCoreSignalRTestModule.cs +++ b/framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/AbpAspNetCoreSignalRTestModule.cs @@ -1,8 +1,11 @@ using System; using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; +using Volo.Abp.AspNetCore.SignalR.SampleHubs; using Volo.Abp.AspNetCore.TestBase; using Volo.Abp.Autofac; using Volo.Abp.Modularity; +using Volo.Abp.Uow; namespace Volo.Abp.AspNetCore.SignalR; @@ -13,6 +16,8 @@ namespace Volo.Abp.AspNetCore.SignalR; )] public class AbpAspNetCoreSignalRTestModule : AbpModule { + public const string UnitOfWorkReservedHeaderName = "X-Unit-Of-Work-Reserved"; + public static Exception UseConfiguredEndpointsException { get; set; } public override void OnApplicationInitialization(ApplicationInitializationContext context) @@ -20,11 +25,22 @@ public override void OnApplicationInitialization(ApplicationInitializationContex var app = context.GetApplicationBuilder(); app.UseRouting(); + app.UseUnitOfWork(); + app.Use(async (httpContext, next) => + { + httpContext.Response.Headers[UnitOfWorkReservedHeaderName] = + (httpContext.RequestServices.GetRequiredService().UnitOfWork?.IsReserved == true).ToString(); + + await next(); + }); UseConfiguredEndpointsException = null; try { - app.UseConfiguredEndpoints(); + app.UseConfiguredEndpoints(endpoints => + { + endpoints.MapHub("/manually-mapped-hub"); + }); } catch (Exception e) { diff --git a/framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/AbpSignalRUnitOfWork_Tests.cs b/framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/AbpSignalRUnitOfWork_Tests.cs new file mode 100644 index 00000000000..a6c9e5d5539 --- /dev/null +++ b/framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/AbpSignalRUnitOfWork_Tests.cs @@ -0,0 +1,38 @@ +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; +using Shouldly; +using Xunit; + +namespace Volo.Abp.AspNetCore.SignalR; + +public class AbpSignalRUnitOfWork_Tests : AbpAspNetCoreTestBase +{ + [Theory] + [InlineData("/signalr-hubs/regular-abp/negotiate?negotiateVersion=1")] + [InlineData("/custom-route-hub/negotiate?negotiateVersion=1")] + [InlineData("/manually-mapped-hub/negotiate?negotiateVersion=1")] + public async Task Should_Not_Reserve_A_Unit_Of_Work_For_The_Hub_Requests(string url) + { + var response = await Client.PostAsync(url, null); + + response.StatusCode.ShouldBe(HttpStatusCode.OK); + IsUnitOfWorkReserved(response).ShouldBeFalse(); + } + + [Theory] + [InlineData("/not-a-hub")] + [InlineData("/custom-route-hub-export")] + public async Task Should_Reserve_A_Unit_Of_Work_For_The_Other_Requests(string url) + { + var response = await Client.GetAsync(url); + + IsUnitOfWorkReserved(response).ShouldBeTrue(); + } + + private static bool IsUnitOfWorkReserved(HttpResponseMessage response) + { + return bool.Parse(response.Headers.GetValues(AbpAspNetCoreSignalRTestModule.UnitOfWorkReservedHeaderName).Single()); + } +} diff --git a/framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/SampleHubs/CustomRouteHub.cs b/framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/SampleHubs/CustomRouteHub.cs new file mode 100644 index 00000000000..7e2f233efce --- /dev/null +++ b/framework/test/Volo.Abp.AspNetCore.SignalR.Tests/Volo/Abp/AspNetCore/SignalR/SampleHubs/CustomRouteHub.cs @@ -0,0 +1,6 @@ +namespace Volo.Abp.AspNetCore.SignalR.SampleHubs; + +[HubRoute("/custom-route-hub")] +public class CustomRouteHub : AbpHub +{ +} diff --git a/framework/test/Volo.Abp.Uow.Tests/Volo/Abp/Uow/UnitOfWork_Ambient_Scope_Tests.cs b/framework/test/Volo.Abp.Uow.Tests/Volo/Abp/Uow/UnitOfWork_Ambient_Scope_Tests.cs index 62395bf8e6c..80de5af8c86 100644 --- a/framework/test/Volo.Abp.Uow.Tests/Volo/Abp/Uow/UnitOfWork_Ambient_Scope_Tests.cs +++ b/framework/test/Volo.Abp.Uow.Tests/Volo/Abp/Uow/UnitOfWork_Ambient_Scope_Tests.cs @@ -71,4 +71,49 @@ public async Task UnitOfWorkManager_Reservation_Test() _unitOfWorkManager.Current.ShouldBeNull(); } + + [Theory] + [InlineData(true, false)] + [InlineData(true, true)] + [InlineData(false, true)] + public async Task Should_Not_Begin_A_Reserved_Unit_Of_Work_That_Is_Not_Alive_Anymore(bool complete, bool dispose) + { + var reservationEnded = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + using (var uow1 = _unitOfWorkManager.Reserve("Reservation1")) + { + // Captures the reserved unit of work as its ambient one and keeps running after the reservation ends, + // like the work that is started by a request and outlives it. + var outlivingWork = Task.Run(async () => + { + await reservationEnded.Task; + + var isBegun = _unitOfWorkManager.TryBeginReserved("Reservation1", new AbpUnitOfWorkOptions()); + + using (var uow2 = _unitOfWorkManager.Begin()) + { + _unitOfWorkManager.Current.ShouldNotBeNull(); + _unitOfWorkManager.Current.Id.ShouldNotBe(uow1.Id); + + await uow2.CompleteAsync(); + } + + return isBegun; + }); + + if (complete) + { + await uow1.CompleteAsync(); + } + + if (dispose) + { + uow1.Dispose(); + } + + reservationEnded.SetResult(); + + (await outlivingWork).ShouldBeFalse(); + } + } }