[WIP] Message context

[AnthonySteele]

A start of an approach to the "message Envelopes" feature.
Do not merge, just please say if you think this is worth completing, or if it is misguided or you can see a better way.

This approach broadens the handler contract again into
Task<bool> HandleAsync(T message, MessageContext context).
We have expanded the contact before for async, moving from bool to Task<bool> return, with a similar wrapper.

The now idea is that

• Simple consumers would use the existing IHandlerAsync.Handle<T> as before and work with the message deserialised as T.
• Consumers that need all the metadata would use IHandlerWithMessageContext instead, and get the additional messageContext object containing various other properties such as the queue url and the raw SQS message.
• The queue url and anything that can be read from the raw SQS message would be deleted from the Message base class. if you need them, that's not a "simple consumer", that's a "consumer that needs the metadata".
• internal to JustSaying, only the IHandlerWithMessageContext need be dealt with, as IHandlerAsync instances can be wrapped in the HandlerAdapter and appear as IHandlerWithMessageContext.
• extension methods over MessageContext could make it easy to read e.g. specific named metadata on the raw SQS Message.
• Longer term, the simpler handler and wrapper might be deprecated and fall away, as with the synchronous handler.

The additional MessageContext object is a parallel to the PublishMetadata in #449.

"Envelope" is also a possible design here, but has the additional implementation complexity that as the Envelope contains a generic message of some type T: Message, the envelope is also generic over T: Message; whereas the MessageContext is not a generic at all.

[martincostello]

If others are happy with this, then before merging needs:

• Tests.
• Examples/docs.
• /// comments.

[AnthonySteele]

.. and it needs to be wired in

• when registering handlers
• when finding handlers

etc

[AnthonySteele]

This plan effectively broadens the handler contract again, adding MessageContext It is fairly simple to implement in JustSaying, and fairly simple to update consumers, but

• The handler always have the context, even when it does not have a use for it, or even know what to do with it. The context contains more "low level" information, which may lead consumers into the wrong patterns.
• JustSaying always has to create the context for each message.

Are there other options?

An alternate proposal is this:

JustSaying defines an interface IMessageContextAccessor with a method MessageContext GetContext()

A Handler class can constructor-inject this interface (but does not have to). And later call it to get the context as needed.

Pros:

• Handlers that don't care about context stay as they are.
• JustSaying won't need to create the context object unless it's actually got.

Cons:

• Simpler handlers stay simple, but complex things become harder. If a handler does want the message context, there are more non-obvious moving parts in order to get it
• This is harder to implement inside JustSaying as it ties the handler instance lifecycle to the message receipt. IMessageContextAccessor would have to be transient in whichever container is in use. Initial investigation raised issue WithMessageHandler overload not helpful #484 for starters. More code and changes needed to make it work, not yet clear exactly how.

[slang25]

Great write-up @AnthonySteele.

IMessageContextAccessor would have to be transient in whichever container is in use

It would be registered as a singleton (because it's a kinda a factory), and we'd do it for them in AddJustSaying.

There's also the pro that if your handler has some dependencies, you could push the IMessageContextAccessor detail into those types, which keeping your handler clean of the extra AWS specific concerns.

As an example, maybe I'd have a IRetryHelper (naming is hard 😳), which might want to know about the attempt number, or visibility timeout. You could have:

class RetryHelper : IRetryHelper
{
    public RetryHelper(IMessageContextAccessor contextAccessor)
   {
       // later on do something with contextAccessor.GetContext().AttemptNumber (or whatever)

Now your handler can depend on IRetryHelper, but not need a IMessageContextAccessor itself.

[AnthonySteele]

It would be registered as a singleton

That underscores that we can't do things this way unless we find out a good way to do that at all. Yes it makes sense to register IMessageContextAccessor as a singleton. However it would be a singleton that has to return the SQS message currently being processed and other transient state.

[slang25]

So what I'm proposing is that we do exactly what aspnetcore does, which backs it by AsyncLocal. It's sort of an ambient context, but with a nice way to get hold of it (via a dependency injected factory). It turns out the lifetime of it would be academic, it's singleton in aspnet purely for performance (very minor).
I have 2/3rds of it done here and will hopefully share it before the weekend.

[AnthonySteele]

FYI, "exactly what aspnetcore does"

[slang25]

Thank you, I had lost that link. Sometimes code says a thousand words 😄

[martincostello]

My thinking on the accessor would be something like one of the two ideas:

Option A - Always present

In this example, the accessor is always in DI and things will break without it.

// Any relevant pre-processing...

var accessor = resolver.GetInstance<IMyAccessorThing>();
accessor.Current = new AwsContextThing();

// All the stuff that processes things...

accessor.Current = null; // We're done with this message completely

Option B - Accessor is optional

In this example, it's opt-in for if you need it (to save allocations etc.) like how IHttpContextAccessor is opt-in.

// On setup
services.AddJustSaying(...)
             .AddJusSayingContextOrWhateverForAwsContextAccessStuff();

// Any relevant pre-processing...

var accessor = resolver.GetInstance<IMyAccessorThing>();

if (accessor != null)
{
    accessor.Current = new AwsContextThing();
}

// All the stuff that processes things...

if (accessor != null)
{
    accessor.Current = null; // We're done with this message completely
}

[AnthonySteele]

In this model IMessageContextAccessor merely a wrapper, the real decision is to put data into the "async thread local storage" just before invoking the message handler or not. We can base this on performance data: if it's cheap then do it every time, if not then have some kind of switch that matches up with the availability of a IMessageContextAccessor as set up by some kind of opt-in.

Like assess to a current http context, this might lead people into bad designs - e.g. with http context, it is better that the controller has the sole responsibility for dealing with http request and responses, and classes that it calls to do domain things do not couple to this by e.g. looking up header values in the current request. However "you can use it to make bad designs" is not a fatal flaw, it is a pitfall to be wary of.

[slang25]

@martincostello not sure I'm fully following you. If we go down the IMessageContextAccessor route, then it is just an abstraction to get hold of it (for testing purposes).

The implementation is static, so we wouldn't need to resolve it in order to set it. We could check to see if IMessageContextAccessor is registered and then opt out of the process (I think this is your point).

We could get something from DI and set the context, but only if we first create a DI scope that we manage.

Perf wise it would be interesting to see how these 2 options do. Not sure it's going to be enough of an impact to influence a decision, but I'll hold judgement for when I see the numbers.

Just to be completely clear, I think there are 3 high-level options:

• Pass the context in to the handle method - no jiggery-pokery
• IMessageContextAccessor with DI scopes
• IMessageContextAccessor with AsyncLocal backing - a la aspnetcore HttpContextAccessor

[martincostello]

What I’m suggesting is kind of 2 and 3 @slang25, I’ve just included a pattern where it’s optional because in ASP.NET Core IHttpContextAccessor is opt-in and pay-for-play, so it’s “static” by being an interface that’s optionally registered in DI.

[AnthonySteele]

Hmm, good points, paring this down to the essentials, the DI aspect is unnecessary. If the data is in async local storage, then a static helper method will be enough to get it.

[slang25]

Ah yes, I'm with you now 👍

[slang25]

I like the optional thing, it seems like a nice pattern. The only downside I can think of is that if a user forgets the registration and is using IMessageContextAccessor then they'd get a NullReferenceException and we couldn't give a taylored message. Feels like a reasonable trade off though.

[AnthonySteele]

Closed in favour of the option discussed in the comments

[martincostello]

The null thing was why the code example had the null checks 😉

[AnthonySteele]

You don't need a IMessageContextAccessor interface to access thread-local static storage. To do so in a typesafe manner all you need is a couple of static methods. However @martincostello makes a good case that putting this behind an interface means it's much easier to mock out for test etc.

[AnthonySteele]

See #486