Skip to content
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

Refactor IMessageDispatcher into Middleware #932

Merged
merged 49 commits into from
Sep 28, 2021
Merged

Refactor IMessageDispatcher into Middleware #932

merged 49 commits into from
Sep 28, 2021

Conversation

gkinsman
Copy link
Contributor

@gkinsman gkinsman commented Aug 7, 2021

This PR partially resolves #875 by moving much of the post-processing code from the MessageDispatcher to separate middlewares.

The default middleware chain for all handlers is now:

"MiddlewareChain": [
  "MessageContextAccessorMiddleware",
  "ErrorHandlerMiddleware",
  "LoggingMiddleware",
  "StopwatchMiddleware",
  "SqsPostProcessorMiddleware",
  "HandlerInvocationMiddleware`1"
]

It is also now possible to modify the middleware configuration from the subscription builders directly, where before you had to go via the SqsReadConfiguration.

Before:

(TopicSubscriptionBuilder<SimpleMessage> c) => 
  c.WithReadConfiguration(rc =>
      rc.WithMiddlewareConfiguration(m =>
        m.UseExactlyOnce<SimpleMessage>("simple-message-lock")))))

After:

(TopicSubscriptionBuilder<SimpleMessage> c) => 
    c.WithMiddlewareConfiguration(m =>
        m.UseExactlyOnce<SimpleMessage>("lock-simple-message")
         .UseDefaults<SimpleMessage>(handler.GetType()))))

The resulting middleware chain:

        "MiddlewareChain": [
          "ExactlyOnceMiddleware`1",
          "MessageContextAccessorMiddleware",
          "ErrorHandlerMiddleware",
          "LoggingMiddleware",
          "StopwatchMiddleware",
          "SqsPostProcessorMiddleware",
          "HandlerInvocationMiddleware`1"
        ]

The UseDefaults method will add all of the above middlewares as a shortcut for having to add them all manually. Previously it wasn't possible to have a no-op pipeline as the HandlerInvocationMiddleware was always added, but now you can by not calling this.

A bit of debt

The need to include the handler type is to maintain the IMessageMonitor.HandlerExecutionTime(_handlerType, context.MessageType, watch.Elapsed) contract. Possibly this call could be moved to the handler invocation middleware where we have the handler type (so that we don't need to resolve the handler in each middleware).

DI Wireup

There has also been a change to how we resolve services from configuration (using ServicesBuilder) vs the provided IServiceResolver. Previously both the IServiceResolver and the ServicesBuilder were checked to see if the user has overridden various services such as IMessageMonitor via config instead of adding it to the container. IMessageMonitor is used all over the place, so this check had to be done in each location (it wasn't, which was causing bugs).

There is now an IServiceResolver over the ServicesBuilder (ServiceBuilderServiceResolver) that enables resolution from anywhere without needing to care where the component was registered. A CompoundServiceResolver provides a single service resolver for both the service builder resolver, and the user provided resolver (or the DefaultServiceResolver if none supplied).

This is particularly useful for middlewares, as previously we had to know about the ServicesBuilder in order to find the correct IMessageMonitor, for example. We can now wholly rely on a single IServicesResolver.

I'm keen for feedback on this design - it's a little ugly, but IMO superior to infecting the rest of the codebase with knowledge of the services builder.

TODO:

  • Add XML docs to new public members
  • Cleanup any dead code/things hanging around
  • Add some more tests for middleware building

@codecov
Copy link

codecov bot commented Aug 7, 2021

Codecov Report

Merging #932 (c9ba1d4) into main (f5250dd) will decrease coverage by 0.02%.
The diff coverage is 92.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #932      +/-   ##
==========================================
- Coverage   84.39%   84.37%   -0.03%     
==========================================
  Files         120      131      +11     
  Lines        3120     3199      +79     
==========================================
+ Hits         2633     2699      +66     
- Misses        487      500      +13     
Flag Coverage Δ
linux 84.12% <92.19%> (-0.15%) ⬇️
macos 52.73% <64.72%> (-0.41%) ⬇️
windows 53.11% <64.72%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...c/JustSaying/Fluent/SqsReadConfigurationBuilder.cs 76.92% <ø> (-6.42%) ⬇️
...ing/Messaging/MessageHandling/MessageAttributes.cs 100.00% <ø> (ø)
...ging/MessageSerialization/MessageWithAttributes.cs 100.00% <ø> (ø)
...aging/MessageSerialization/NewtonsoftSerializer.cs 100.00% <ø> (ø)
...g/MessageSerialization/SystemTextJsonSerializer.cs 100.00% <ø> (ø)
...ng/Middleware/ExactlyOnce/ExactlyOnceMiddleware.cs 77.77% <ø> (-14.82%) ⬇️
...ware/Metrics/MetricsMiddlewareBuilderExtensions.cs 100.00% <ø> (ø)
...Saying/Fluent/QueueAddressSubscriptionBuilder`1.cs 83.33% <50.00%> (-5.24%) ⬇️
...rc/JustSaying/Fluent/QueueSubscriptionBuilder`1.cs 88.33% <50.00%> (-3.34%) ⬇️
src/JustSaying/Fluent/QueueAddressQueue.cs 80.95% <60.00%> (-7.94%) ⬇️
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5250dd...c9ba1d4. Read the comment docs.

@gkinsman gkinsman changed the title Fix messagebackoffstrategy Refactor IMessageDispatcher into Middleware Aug 7, 2021
{
if (builder == null) throw new ArgumentNullException(nameof(builder));

IMessageMonitor monitor = builder.ServiceResolver.ResolveService<IMessageMonitor>();
Copy link
Contributor Author

@gkinsman gkinsman Aug 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't just resolve the ErrorHandlerMiddleware from the resolver, because if the IMessageMonitor was added via the ServicesBuilder, then resolving the middleware from the resolver will bypass the SB and resolve from the container, which is a no-op by default.

The downside is that middlewares that do this are instance-per-queue instead of shared.

George Kinsman added 15 commits September 8, 2021 23:13
- Adjusted the handler builder model to make applying user overrides to defaults more straightforward. The previous method of providing a Configure func worked until I tried to apply defaults beforehand that could be adjusted, and it became impossible to ensure they were ordered correctly. With the new way, defaults are applied unless users provide a middleware configuration override, in which case they must call UseDefaults:
```
.ConfigureJustSaying((builder) =>
    builder.WithLoopbackTopic<SimpleMessage>(UniqueName,
        c => c.WithMiddlewareConfiguration(m =>
        {
            m.UseExactlyOnce<SimpleMessage>("lock-simple-message");
            m.UseDefaults<SimpleMessage>(handler.GetType());
        })))
```
- Removed the concept of clearing the middleware chain, as everything is now opt-in
- Made sure all approval test results were prefixed by their classname to make them easier to find :-)
- Fix up some stray approval tests - found an easier way to work around the lambda function name in approval test filename problem.
- Minor cleanup
- Add new AwaitableMiddleware that can be used to wait for an entire middleware chain to complete instead of just the inner handler. Useful for testing middleware after effects.
- BackoffMiddleware can be singleton as the exception is now stored on the handle context
- DefaultServiceResolver can be a bit smarter
- Added some docs, applied some code conventions
- Add try/finally around backoff middleware - better safe than sorry if it's used elsewhere.
- Made Client private in ISqsQueue to hide it to make testing easier. It's an implementation detail. Added methods we use to ISqsQueue
- Refactoring tests that use FakeAmazonSqs to use FakeSqsQueue instead. Data driven tests!
- Remove DummySqsQueue as it does the same job as FakeSqsQueue
var messages = PopMessagesFromSourceQueue(sourceQueue);
var receiptHandles = messages.ToDictionary(m => m.MessageId, m => m.ReceiptHandle);

var sendResponse = destinationQueue.Client.SendMessageBatch(new SendMessageBatchRequest
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Client property is private now as we no longer expose the IAmazonSqs, so I've removed this command as I don't think its used very much. We have other ways of handling retries.

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.

I wonder if with the extra CancellationToken usages if anywhere needs changing to more gracefully needs to handle if it fires and causes a cancellation mid operation? (Say between two API calls related to a single message batch, like a receive and then a delete)

src/JustSaying/AwsTools/MessageHandling/ISqsQueue.cs Outdated Show resolved Hide resolved
src/JustSaying/AwsTools/MessageHandling/ISqsQueue.cs Outdated Show resolved Hide resolved
src/JustSaying/AwsTools/MessageHandling/ISqsQueue.cs Outdated Show resolved Hide resolved
src/JustSaying/AwsTools/MessageHandling/ISqsQueue.cs Outdated Show resolved Hide resolved
src/JustSaying/AwsTools/MessageHandling/ISqsQueue.cs Outdated Show resolved Hide resolved
src/JustSaying/AwsTools/MessageHandling/ISqsQueue.cs Outdated Show resolved Hide resolved
src/JustSaying/AwsTools/MessageHandling/ISqsQueue.cs Outdated Show resolved Hide resolved
src/JustSaying/AwsTools/MessageHandling/SqsQueueReader.cs Outdated Show resolved Hide resolved
src/JustSaying/Extensions/AmazonSqsClientExtensions.cs Outdated Show resolved Hide resolved
gkinsman and others added 4 commits September 9, 2021 09:26
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments here and there. Most of it us just me being nit-picky about /// docs and null checks on the public members.

martincostello
martincostello previously approved these changes Sep 23, 2021
@martincostello martincostello added this to the v7.0.0 milestone Sep 23, 2021
Remove IMessageBackoffStrategy from DI
@gkinsman gkinsman merged commit afcd890 into justeattakeaway:main Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add back some of the extensibility to JustSaying 7 APIs
3 participants