From 1c3fa6c582fc1e8847e5462f1720fdee63c479d2 Mon Sep 17 00:00:00 2001 From: George Kinsman Date: Wed, 10 Nov 2021 16:31:29 +0000 Subject: [PATCH 1/6] - Ensure middlewares are registered as transient so that each middleware pipeline gets its own instance. Without this, when multiple subscriptions are registered, middlewares clobber each other when built. --- .../IServiceCollectionExtensions.cs | 6 +-- .../Handle/HandlerMiddlewareBuilder.cs | 5 +- .../Messaging/Middleware/MiddlewareBase`2.cs | 4 +- .../WhenSubscribingToMultipleTopics.cs | 48 +++++++++++++++++++ 4 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 tests/JustSaying.IntegrationTests/Fluent/Subscribing/WhenSubscribingToMultipleTopics.cs diff --git a/src/JustSaying.Extensions.DependencyInjection.Microsoft/IServiceCollectionExtensions.cs b/src/JustSaying.Extensions.DependencyInjection.Microsoft/IServiceCollectionExtensions.cs index 82263438c..0a2785f4b 100644 --- a/src/JustSaying.Extensions.DependencyInjection.Microsoft/IServiceCollectionExtensions.cs +++ b/src/JustSaying.Extensions.DependencyInjection.Microsoft/IServiceCollectionExtensions.cs @@ -116,7 +116,7 @@ public static IServiceCollection AddJustSaying(this IServiceCollection services, { throw new ArgumentNullException(nameof(configure)); } - + // Register as self so the same singleton instance implements two different interfaces services.TryAddSingleton((p) => new ServiceProviderResolver(p)); services.TryAddSingleton((p) => p.GetRequiredService()); @@ -127,8 +127,8 @@ public static IServiceCollection AddJustSaying(this IServiceCollection services, services.TryAddSingleton(); services.TryAddSingleton(); - services.TryAddSingleton(); - services.TryAddSingleton(); + services.TryAddTransient(); + services.TryAddTransient(); services.AddSingleton(); services.TryAddSingleton(serviceProvider => serviceProvider.GetRequiredService()); diff --git a/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs b/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs index 086f5090b..9ab580a74 100644 --- a/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs +++ b/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs @@ -40,6 +40,9 @@ public HandlerMiddlewareBuilder(IHandlerResolver handlerResolver, IServiceResolv /// The current HandlerMiddlewareBuilder. public HandlerMiddlewareBuilder Use() where TMiddleware : MiddlewareBase { + var newMiddleware = ServiceResolver.ResolveService(); + if (newMiddleware.HasNext) throw new InvalidOperationException("Middlewares must be registered as Transient"); + _middlewares.Add(() => ServiceResolver.ResolveService()); return this; } @@ -140,4 +143,4 @@ public HandleMessageMiddleware Build() return MiddlewareBuilder.BuildAsync(middlewares.ToArray()); } -} \ No newline at end of file +} diff --git a/src/JustSaying/Messaging/Middleware/MiddlewareBase`2.cs b/src/JustSaying/Messaging/Middleware/MiddlewareBase`2.cs index 16160abab..bee350758 100644 --- a/src/JustSaying/Messaging/Middleware/MiddlewareBase`2.cs +++ b/src/JustSaying/Messaging/Middleware/MiddlewareBase`2.cs @@ -10,6 +10,8 @@ public MiddlewareBase WithNext(MiddlewareBase ne return this; } + public bool HasNext => _next != null; + public async Task RunAsync( TContext context, Func> func, @@ -51,4 +53,4 @@ internal IEnumerable Interrogate() yield return middlewareName; } } -} \ No newline at end of file +} diff --git a/tests/JustSaying.IntegrationTests/Fluent/Subscribing/WhenSubscribingToMultipleTopics.cs b/tests/JustSaying.IntegrationTests/Fluent/Subscribing/WhenSubscribingToMultipleTopics.cs new file mode 100644 index 000000000..36d473291 --- /dev/null +++ b/tests/JustSaying.IntegrationTests/Fluent/Subscribing/WhenSubscribingToMultipleTopics.cs @@ -0,0 +1,48 @@ +using System.Threading.Tasks; +using JustSaying.Messaging.MessageHandling; +using JustSaying.TestingFramework; +using Microsoft.Extensions.DependencyInjection; +using Shouldly; +using Xunit.Abstractions; + +namespace JustSaying.IntegrationTests.Fluent.Subscribing +{ + public class WhenSubscribingToMultipleTopics : IntegrationTestBase + { + public WhenSubscribingToMultipleTopics(ITestOutputHelper outputHelper) : base(outputHelper) + { } + + [AwsFact] + public async Task Then_Both_Handlers_Receive_Messages() + { + // Arrange + var genericHandler = new InspectableHandler>(); + var nonGenericHandler = new InspectableHandler(); + + var services = GivenJustSaying() + .ConfigureJustSaying((builder) => builder.WithLoopbackTopic>($"{UniqueName}-generic")) + .ConfigureJustSaying((builder) => builder.WithLoopbackTopic($"{UniqueName}-nongeneric")) + .AddSingleton>>(genericHandler) + .AddSingleton>(nonGenericHandler); + + await WhenAsync( + services, + async (publisher, listener, serviceProvider, cancellationToken) => + { + await listener.StartAsync(cancellationToken); + await publisher.StartAsync(cancellationToken); + + // Act + await publisher.PublishAsync(new GenericMessage(), cancellationToken); + await publisher.PublishAsync(new SimpleMessage(), cancellationToken); + + await Patiently.AssertThatAsync(OutputHelper, + () => + { + genericHandler.ReceivedMessages.ShouldHaveSingleItem(); + nonGenericHandler.ReceivedMessages.ShouldHaveSingleItem(); + }); + }); + } + } +} From 12063ce03279b0c684d142a324ac99d5e56ec349 Mon Sep 17 00:00:00 2001 From: George Kinsman Date: Wed, 10 Nov 2021 17:01:46 +0000 Subject: [PATCH 2/6] - Update registrations for structure map too --- .../JustSayingRegistry.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/JustSaying.Extensions.DependencyInjection.StructureMap/JustSayingRegistry.cs b/src/JustSaying.Extensions.DependencyInjection.StructureMap/JustSayingRegistry.cs index be67b79ca..72222c7b2 100644 --- a/src/JustSaying.Extensions.DependencyInjection.StructureMap/JustSayingRegistry.cs +++ b/src/JustSaying.Extensions.DependencyInjection.StructureMap/JustSayingRegistry.cs @@ -38,8 +38,8 @@ public JustSayingRegistry() For().Use(context => context.GetInstance()); For().Use(context => context.GetInstance()); - For().Singleton(); - For().Singleton(); + For().Transient(); + For().Transient(); For() .Use( @@ -56,4 +56,4 @@ public JustSayingRegistry() For().Use(context => context.GetInstance()); For().Use(context => context.GetInstance()); } -} \ No newline at end of file +} From e28e8fea59b2a41e9de66ecd3acd27f20c0ef5bb Mon Sep 17 00:00:00 2001 From: George Kinsman Date: Wed, 10 Nov 2021 17:24:47 +0000 Subject: [PATCH 3/6] - Reuse already created middleware instance --- .../Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs b/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs index 9ab580a74..54937cb38 100644 --- a/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs +++ b/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs @@ -43,7 +43,7 @@ public HandlerMiddlewareBuilder Use() where TMiddleware : Middlewar var newMiddleware = ServiceResolver.ResolveService(); if (newMiddleware.HasNext) throw new InvalidOperationException("Middlewares must be registered as Transient"); - _middlewares.Add(() => ServiceResolver.ResolveService()); + _middlewares.Add(() => newMiddleware); return this; } From 21efe1042a0cc69d65ae6b5fcfd76c4a5649dc8f Mon Sep 17 00:00:00 2001 From: George Kinsman Date: Wed, 10 Nov 2021 17:26:32 +0000 Subject: [PATCH 4/6] - Add docs for new thrown exception. --- .../Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs b/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs index 54937cb38..90a2d835b 100644 --- a/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs +++ b/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs @@ -38,10 +38,11 @@ public HandlerMiddlewareBuilder(IHandlerResolver handlerResolver, IServiceResolv /// /// The type of the middleware to add. /// The current HandlerMiddlewareBuilder. + /// When the middleware is not registered as Transient, an exception will be thrown if the resolved middleware is already part of a pipeline. public HandlerMiddlewareBuilder Use() where TMiddleware : MiddlewareBase { var newMiddleware = ServiceResolver.ResolveService(); - if (newMiddleware.HasNext) throw new InvalidOperationException("Middlewares must be registered as Transient"); + if (newMiddleware.HasNext) throw new InvalidOperationException("Middlewares must be registered as Transient."); _middlewares.Add(() => newMiddleware); return this; From 3f0e068170a2805801e045d2fbb2c13d904d9e86 Mon Sep 17 00:00:00 2001 From: George Kinsman Date: Wed, 10 Nov 2021 17:41:54 +0000 Subject: [PATCH 5/6] - Make HasNext internal as its only used by us - File scoped namespaces! --- .../Messaging/Middleware/MiddlewareBase`2.cs | 2 +- .../WhenSubscribingToMultipleTopics.cs | 65 +++++++++---------- 2 files changed, 33 insertions(+), 34 deletions(-) diff --git a/src/JustSaying/Messaging/Middleware/MiddlewareBase`2.cs b/src/JustSaying/Messaging/Middleware/MiddlewareBase`2.cs index bee350758..2cd3d111a 100644 --- a/src/JustSaying/Messaging/Middleware/MiddlewareBase`2.cs +++ b/src/JustSaying/Messaging/Middleware/MiddlewareBase`2.cs @@ -10,7 +10,7 @@ public MiddlewareBase WithNext(MiddlewareBase ne return this; } - public bool HasNext => _next != null; + internal bool HasNext => _next != null; public async Task RunAsync( TContext context, diff --git a/tests/JustSaying.IntegrationTests/Fluent/Subscribing/WhenSubscribingToMultipleTopics.cs b/tests/JustSaying.IntegrationTests/Fluent/Subscribing/WhenSubscribingToMultipleTopics.cs index 36d473291..76ac2f1f1 100644 --- a/tests/JustSaying.IntegrationTests/Fluent/Subscribing/WhenSubscribingToMultipleTopics.cs +++ b/tests/JustSaying.IntegrationTests/Fluent/Subscribing/WhenSubscribingToMultipleTopics.cs @@ -5,44 +5,43 @@ using Shouldly; using Xunit.Abstractions; -namespace JustSaying.IntegrationTests.Fluent.Subscribing +namespace JustSaying.IntegrationTests.Fluent.Subscribing; + +public class WhenSubscribingToMultipleTopics : IntegrationTestBase { - public class WhenSubscribingToMultipleTopics : IntegrationTestBase - { - public WhenSubscribingToMultipleTopics(ITestOutputHelper outputHelper) : base(outputHelper) - { } + public WhenSubscribingToMultipleTopics(ITestOutputHelper outputHelper) : base(outputHelper) + { } - [AwsFact] - public async Task Then_Both_Handlers_Receive_Messages() - { - // Arrange - var genericHandler = new InspectableHandler>(); - var nonGenericHandler = new InspectableHandler(); + [AwsFact] + public async Task Then_Both_Handlers_Receive_Messages() + { + // Arrange + var genericHandler = new InspectableHandler>(); + var nonGenericHandler = new InspectableHandler(); - var services = GivenJustSaying() - .ConfigureJustSaying((builder) => builder.WithLoopbackTopic>($"{UniqueName}-generic")) - .ConfigureJustSaying((builder) => builder.WithLoopbackTopic($"{UniqueName}-nongeneric")) - .AddSingleton>>(genericHandler) - .AddSingleton>(nonGenericHandler); + var services = GivenJustSaying() + .ConfigureJustSaying((builder) => builder.WithLoopbackTopic>($"{UniqueName}-generic")) + .ConfigureJustSaying((builder) => builder.WithLoopbackTopic($"{UniqueName}-nongeneric")) + .AddSingleton>>(genericHandler) + .AddSingleton>(nonGenericHandler); - await WhenAsync( - services, - async (publisher, listener, serviceProvider, cancellationToken) => - { - await listener.StartAsync(cancellationToken); - await publisher.StartAsync(cancellationToken); + await WhenAsync( + services, + async (publisher, listener, serviceProvider, cancellationToken) => + { + await listener.StartAsync(cancellationToken); + await publisher.StartAsync(cancellationToken); - // Act - await publisher.PublishAsync(new GenericMessage(), cancellationToken); - await publisher.PublishAsync(new SimpleMessage(), cancellationToken); + // Act + await publisher.PublishAsync(new GenericMessage(), cancellationToken); + await publisher.PublishAsync(new SimpleMessage(), cancellationToken); - await Patiently.AssertThatAsync(OutputHelper, - () => - { - genericHandler.ReceivedMessages.ShouldHaveSingleItem(); - nonGenericHandler.ReceivedMessages.ShouldHaveSingleItem(); - }); - }); - } + await Patiently.AssertThatAsync(OutputHelper, + () => + { + genericHandler.ReceivedMessages.ShouldHaveSingleItem(); + nonGenericHandler.ReceivedMessages.ShouldHaveSingleItem(); + }); + }); } } From 48bd39be75854ed18a2e4b8fd3d77f2178b1e7ee Mon Sep 17 00:00:00 2001 From: George Kinsman Date: Wed, 10 Nov 2021 19:16:30 +0000 Subject: [PATCH 6/6] - Add more descriptive error message for when user middlewares are registered incorrectly --- .../Middleware/Handle/HandlerMiddlewareBuilder.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs b/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs index 90a2d835b..58611914f 100644 --- a/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs +++ b/src/JustSaying/Messaging/Middleware/Handle/HandlerMiddlewareBuilder.cs @@ -42,7 +42,13 @@ public HandlerMiddlewareBuilder(IHandlerResolver handlerResolver, IServiceResolv public HandlerMiddlewareBuilder Use() where TMiddleware : MiddlewareBase { var newMiddleware = ServiceResolver.ResolveService(); - if (newMiddleware.HasNext) throw new InvalidOperationException("Middlewares must be registered as Transient."); + if (newMiddleware.HasNext) + { + throw new InvalidOperationException( + @"Middlewares must be registered into your DI container such that each resolution creates a new instance. +For StructureMap use Transient(), and for Microsoft.Extensions.DependencyInjection, use AddTransient(). +Please check the documentation for your container for more details."); + } _middlewares.Add(() => newMiddleware); return this;