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

Implement Middleware for Handlers #809

Merged
merged 44 commits into from
Dec 8, 2020
Merged

Implement Middleware for Handlers #809

merged 44 commits into from
Dec 8, 2020

Conversation

gkinsman
Copy link
Contributor

@gkinsman gkinsman commented Oct 20, 2020

This PR implements a middleware layer around handlers. Middlewares have a number of advantages over existing solutions:

  • User provided middlewares can be provided via extension methods packages in nuget packages
  • They are composable with other middlewares
  • They don't require any custom code in or around IHandlerAsync<T>
  • They are cancellable with CancellationToken, whereas IHandlerAsync<T> cannot change to add one (or at least we haven't decided to do that)
  • They create a nice place for users to add in cross cutting code, like custom stats, retry strategies, locking, etc etc without needing to change JustSaying.

This work is largely based on the existing middleware code added by @maisiesadler that is used in the MessageReceiveBuffer's receive pipeline.

Some example code

Middlewares can be pulled from DI, or passed in explicitly:

var callRecord = new List<string>();

void Before(string id) => callRecord.Add($"Before_{id}");
void After(string id) => callRecord.Add($"After_{id}");

var outerMiddleware = new TrackingMiddleware("outer", Before, After);
var middleMiddleware = new TrackingMiddleware("middle", Before, After);
var innerMiddleware = new TrackingMiddleware("inner", Before, After);

var services = GivenJustSaying()
    .AddSingleton(outerMiddleware)
    .AddSingleton<IHandlerAsync<SimpleMessage>>(handler)
    .ConfigureJustSaying(builder => builder.WithLoopbackTopic<SimpleMessage>(UniqueName,
        topic => topic.WithMiddlewareConfiguration(app =>
        {
            app.Use<TrackingMiddleware>(); // from DI
            app.Use(() => middleMiddleware); // provide a Func<MiddlewareBase<HandleMessageContext, bool>
            app.Use(innerMiddleware); // Existing instance
        })));

Produces:

Before_outer
Before_middle
Before_inner
After_inner
After_middle
After_outer

Implementation of an extension method that makes a UseExactlyOnce method available to the pipeline, and resolves some services from DI

public static HandlerMiddlewareBuilder UseExactlyOnce<TMessage>(
    this HandlerMiddlewareBuilder builder,
    string lockKey,
    TimeSpan? lockDuration = null)
{
    HandleMessageMiddleware CreateMiddleware()
    {
        var messageLock = builder.ServicesBuilder?.MessageLock?.Invoke() ?? builder.ServiceResolver.ResolveService<IMessageLockAsync>();
        var logger = builder.ServiceResolver.ResolveService<ILogger<ExactlyOnceMiddleware<TMessage>>>();

        return new ExactlyOnceMiddleware<TMessage>(messageLock,
            lockDuration ?? TimeSpan.MaxValue,
            lockKey,
            logger);
    }

    builder.Use(CreateMiddleware);

    return builder;
}

Usage of UseExactlyOnce

.ConfigureJustSaying((builder) =>
    builder.WithLoopbackTopic<SimpleMessage>(UniqueName,
        c =>
            c.WithMiddlewareConfiguration(m =>
                m.UseExactlyOnce<SimpleMessage>("simple-message-lock"))))

They can be chained:

.Configure(pipe =>
                {
                    pipe.Use(m1);
                    pipe.Use(m2);
                    pipe.Use(m3);
                })

@codecov
Copy link

codecov bot commented Oct 20, 2020

Codecov Report

Merging #809 (2ad8bb0) into master (a78936d) will decrease coverage by 0.05%.
The diff coverage is 87.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #809      +/-   ##
==========================================
- Coverage   81.45%   81.39%   -0.06%     
==========================================
  Files         109      113       +4     
  Lines        2804     2838      +34     
==========================================
+ Hits         2284     2310      +26     
- Misses        520      528       +8     
Flag Coverage Δ
linux 81.25% <87.64%> (-0.21%) ⬇️
macos 48.15% <67.05%> (-0.02%) ⬇️
windows 48.00% <67.05%> (-0.13%) ⬇️

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

Impacted Files Coverage Δ
src/JustSaying/Fluent/ServicesBuilder.cs 60.86% <ø> (-4.52%) ⬇️
...criptionGroups/SubscriptionGroupSettingsBuilder.cs 86.20% <ø> (ø)
...aging/Middleware/Receive/ReceiveMessagesContext.cs 66.66% <ø> (ø)
...stSaying/Messaging/Monitoring/MonitorExtensions.cs 100.00% <ø> (ø)
...eware/Handle/HandlerMiddlewareBuilderExtensions.cs 33.33% <33.33%> (ø)
...iddleware/Handle/HandleMessageContextExtensions.cs 50.00% <50.00%> (ø)
...AwsTools/MessageHandling/Dispatch/MiddlewareMap.cs 62.96% <62.96%> (ø)
...trics/MetricsHandlerMiddlewareBuilderExtensions.cs 66.66% <66.66%> (ø)
...ng/Middleware/ExactlyOnce/ExactlyOnceMiddleware.cs 85.18% <71.42%> (ø)
...e/ExactlyOnceHandlerMiddlewareBuilderExtensions.cs 80.00% <80.00%> (ø)
... and 33 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 a78936d...2ad8bb0. Read the comment docs.

@gkinsman
Copy link
Contributor Author

@martincostello can we target this for post-beta? I'm really keen to get the beta process rolling so we can get some features using the new backend.

@martincostello
Copy link
Member

@martincostello can we target this for post-beta? I'm really keen to get the beta process rolling so we can get some features using the new backend.

We can have as many alpha/beta/gamma/whatevers as we want before release internally, and we have the ProGet feed we can host packages in, so I don't think it really matters what label we give it or whatever the release cadence is (or the need to be super-strict on SemVer).

I'd only worry about these distinctions for putting something in NuGet.org, and I think we'd want to be fairly confidently "done" and happy it's working before publishing a beta there.

@gkinsman
Copy link
Contributor Author

@martincostello can we target this for post-beta? I'm really keen to get the beta process rolling so we can get some features using the new backend.

We can have as many alpha/beta/gamma/whatevers as we want before release internally, and we have the ProGet feed we can host packages in, so I don't think it really matters what label we give it or whatever the release cadence is (or the need to be super-strict on SemVer).

I'd only worry about these distinctions for putting something in NuGet.org, and I think we'd want to be fairly confidently "done" and happy it's working before publishing a beta there.

That makes perfect sense, thanks 👍. The main reason for a beta was so that we can switch to a package feed instead of local packages, so as you say we can just use proget 🙂.

gkinsman and others added 21 commits November 11, 2020 12:52
- Most of the way there, just some tests to go
- Consumer API still needs a little TLC
…ndlers that enables extensibility around handlers, replaces HandlerMap

- Reimplements ExactlyOnce as middleware instead of a wrapped handler
- Adds configuration API that enables MVC-like configuration of the handler pipeline
- Lower log level of multiplexer subscriber so its less noisy on startup when running with debug logs
- Instead of passing the next middleware via ctor, do it via WithNext on MiddlewareBase so that we can pull middlewares from DI
- Add new line to make middleware clear
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>
…er.cs

Co-authored-by: Martin Costello <martin@martincostello.com>
…dleware.cs

Co-authored-by: Martin Costello <martin@martincostello.com>
…dleware.cs

Co-authored-by: Martin Costello <martin@martincostello.com>
…dleware.cs

Co-authored-by: Martin Costello <martin@martincostello.com>
…eware.cs

Co-authored-by: Martin Costello <martin@martincostello.com>
…ilderExtensions.cs

Co-authored-by: Martin Costello <martin@martincostello.com>
- Rename resolver to handlerResolver to clearly differentiate between the two
…ilderExtensions.cs

Co-authored-by: Martin Costello <martin@martincostello.com>
- Update tests to use ExactlyOnce middleware
- Lots of docs for middleware extensions
- Split middleware extension methods into folders
- Make middleware map return constructed middleware instead of a func
@gkinsman gkinsman marked this pull request as ready for review November 13, 2020 15:48
@gkinsman gkinsman requested a review from a team as a code owner November 13, 2020 15:48
@ghost ghost requested a review from maurofranchi November 13, 2020 15:48
@gkinsman
Copy link
Contributor Author

This is now ready for review! I've documented the public interfaces, added tests, and removed the old exactly-once and stopwatch handlers and reimplemented as middlewares.

Copy link
Member

@slang25 slang25 left a comment

Choose a reason for hiding this comment

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

Some minor comments, overall a really good addition 😃

- Push Shouldly back up to 4 now that the type load exception is fixed
- Fix some docs
slang25
slang25 previously approved these changes Nov 17, 2020
- Rename some variables for clarity
gkinsman and others added 2 commits December 8, 2020 14:02
…dlerMiddlewareBuilderExtensions.cs

Co-authored-by: Martin Costello <martin@martincostello.com>
@gkinsman gkinsman merged commit 8f8af0a into justeattakeaway:master Dec 8, 2020
@slang25 slang25 mentioned this pull request Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants