pardahlman / RawRabbit

A modern .NET framework for communication over RabbitMq
MIT License
747 stars 144 forks source link

[Discussion] Create default (empty) message context in MessageContextProviderBase.ExtractContext() if it is missed #234

Closed fennekin23 closed 7 years ago

fennekin23 commented 7 years ago

Now code of MessageContextProviderBase.ExtractContext() looks like:

public TMessageContext ExtractContext(object o)
{
    if (o == null)
    {
        return default(TMessageContext);
    }   
    // ...
}

but problem exist when other system pushing messages to the queue using native rabbit mq client and no context is added to them. As the result we can't use context on consumer inside subscription method (because it is null). My suggestion is to change code to the next:

public TMessageContext ExtractContext(object o)
{
    if (o == null)
    {
        return CreateMessageContext();
    }   
    // ...
}

in case of missed message context create new empty instance. So it can be used on consumer side for example for retries:

if (context.RetryInfo.NumberOfRetries > 10)
{
    throw new Exception($"Unable to handle message '{context.GlobalRequestId}'.");
}
pardahlman commented 7 years ago

Hello @sachokFoX 👋 - thanks for kicking of the discussion!

The only reason I see for create a new Message Context is to be able to wire up the AdvancedMessageContext features. Putting features like Retry and Nack in the message context was perhaps not a great design decision in hindsight, just for the scenario that you describe. Conceptually, the message context and the ability to Nack the message are two separate things. In the upcoming version 2.0, the different types of acknowledgement can be returned from the message handler, see AcknowledgementSubscribeTests

await subscriber.SubscribeAsync<BasicMessage>(async recieved =>
{
    // your code here
    return new Ack();
});

Note that the message context itself is not mandatory (might make more sense in a setup with different types of clients).

Also, when I think about it, there might be scenarios where a new message context is confusing or perhaps even wrong. For example: If a custom message context has a bool property DontDeleteEverything (just as an example 😉 ), it would default to false, which might have sever consequences.

As you can hear, I'm leaning against not changing the default behavior - but I'm open to hear what you have to say. Nevertheless, you could easily tweak the configuration and register a custom IMessageContextProvider that creates a new message context if it does not exist.

fennekin23 commented 7 years ago

Hello @pardahlman! Thank you for reply. I understand and agree with you. I'll implement IMessageContextProvider just for my case.