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

Always process messages that have handlers regardless of conventions #5342

Merged
merged 5 commits into from Mar 19, 2019

Conversation

@andreasohlund
Copy link
Member

andreasohlund commented Mar 18, 2019

To avoid messages going to the error queue when there are issues with conventions we should process messages if there are handlers for it regardless of conventions.

@andreasohlund andreasohlund requested a review from Mar 18, 2019
Copy link
Member

timbussmann left a comment

LGTM

@bording

This comment has been minimized.

Copy link
Member

bording commented Mar 18, 2019

This seems like a decent change, but I'm wondering about the behavior change here and what version we'd actually be able to do this in.

For example, this change also means that we'd start autosubscribing to events that weren't previously if they didn't have conventions defined for that event type.

@andreasohlund

This comment has been minimized.

Copy link
Member Author

andreasohlund commented Mar 18, 2019

Went through doco like https://docs.particular.net/nservicebus/messaging/messages-events-commands and I couldn't find any place where we state that a message needs to match a convention to be processed.

Only use case I can dream up for this to be breaking is users that for some reason would remove messages from conventions to "disable" handling it and force it to the error queue but that seems to far fetched?

@andreasohlund

This comment has been minimized.

Copy link
Member Author

andreasohlund commented Mar 18, 2019

For example, this change also means that we'd start autosubscribing to events that weren't previously if they didn't have conventions defined for that event type.

I don't think that is correct since autosubscribe only iterates over types that matches the IsEvent convention:

https://github.com/Particular/NServiceBus/blob/develop/src/NServiceBus.Core/Routing/AutomaticSubscriptions/AutoSubscribe.cs#L47

@andreasohlund

This comment has been minimized.

Copy link
Member Author

andreasohlund commented Mar 18, 2019

@bording I also noted that the handler registry filters out non matching message types

https://github.com/Particular/NServiceBus/blob/develop/src/NServiceBus.Core/Unicast/MessageHandlerRegistry.cs#L63

so autosubscribe should definitely not be affected.

But that makes me wonder if we should remove that check as well and only lean on the IsEvent check? cc @timbussmann

@timbussmann

This comment has been minimized.

Copy link
Member

timbussmann commented Mar 18, 2019

But that makes me wonder if we should remove that check as well and only lean on the IsEvent check? cc @timbussmann

probably yes given the xml doco of the method:

Lists all message type for which we have handlers.

@bording

This comment has been minimized.

Copy link
Member

bording commented Mar 18, 2019

I don't think that is correct since autosubscribe only iterates over types that matches the IsEvent convention:

Ah yes, I missed that line. I was thinking that the code being removed here was the guard against that.

That makes me much less concerned that someone could be depending on the existing behavior here to "disable" a handler but still have the handler code in the endpoint.

@bording

This comment has been minimized.

Copy link
Member

bording commented Mar 18, 2019

But that makes me wonder if we should remove that check as well and only lean on the IsEvent check

If that gets removed, then MessageHandlerRegistry no longer needs conventions passed in at all.

@andreasohlund

This comment has been minimized.

Copy link
Member Author

andreasohlund commented Mar 18, 2019

Check in GetMessageTypes now removed as well

@andreasohlund andreasohlund changed the title Always process messages that have handlers Always process message if there is a handler for it Mar 18, 2019
@andreasohlund andreasohlund added this to the 7.2.0 milestone Mar 18, 2019
@@ -13,11 +13,6 @@
/// </summary>
public class MessageHandlerRegistry
{
internal MessageHandlerRegistry(Conventions conventions)

This comment has been minimized.

Copy link
@bording

bording Mar 18, 2019

Member

Need to keep an internal constructor around to prevent the default public constructor from being added.

This comment has been minimized.

Copy link
@andreasohlund

andreasohlund Mar 18, 2019

Author Member

Not sure why we made the ctor internal back in the day. Do we see any drawback in exposing the public ctor?

This comment has been minimized.

Copy link
@bording

bording Mar 18, 2019

Member

I don't suppose there is any problem with allowing this. You'll need to update the API approval test, then.

@andreasohlund

This comment has been minimized.

Copy link
Member Author

andreasohlund commented Mar 19, 2019

This should be good to go now. I've tried to clean up the title and description for inclusion in the release notes.

Once merged I'll add this to a draft 7.2.0 announcement

@@ -1,4 +1,4 @@
[assembly: System.CLSCompliantAttribute(true)]
[assembly: System.CLSCompliantAttribute(true)]

This comment has been minimized.

Copy link
@andreasohlund

andreasohlund Mar 19, 2019

Author Member

There was some whitespace removed locally

@andreasohlund andreasohlund changed the title Always process message if there is a handler for it Always process messages that have handlers regardless of conventions Mar 19, 2019
@timbussmann timbussmann merged commit 264d350 into develop Mar 19, 2019
6 checks passed
6 checks passed
Compile Finished TeamCity Build NServiceBus / Core / 1. Compile : Running
Details
Inspections Finished TeamCity Build NServiceBus / Core / 2. Inspections : Running
Details
Test .NET Core on Linux Finished TeamCity Build NServiceBus / Core / 3.3 Test (.NET Core on Linux) : Tests passed: 1148, ignored: 43
Details
Test .NET Core on Windows Finished TeamCity Build NServiceBus / Core / 3.2 Test (.NET Core on Windows) : Tests passed: 1148, ignored: 43
Details
Test .NET Framework on Windows Finished TeamCity Build NServiceBus / Core / 3.1 Test (Framework on Windows) : Tests passed: 1382, ignored: 96
Details
WIP Ready for review
Details
@timbussmann timbussmann deleted the process-messages-with-handlers branch Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.