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

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

Closed
fennekin23 opened this issue Jun 8, 2017 · 2 comments

Comments

@fennekin23
Copy link

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
Copy link
Owner

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
Copy link
Author

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

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

No branches or pull requests

2 participants