Fix ref struct scan for sagas #7187
Conversation
| // Even though it is not a message type, it is still checked by passing it to conventions. | ||
| // The conventions added by Sagas were throwing an exception when passed a ref struct. | ||
| // See https://github.com/Particular/NServiceBus/issues/7179 for details. | ||
| ref struct RefStruct { } |
There was a problem hiding this comment.
I think this is good enough since the all acceptance tests scope their type scanning to only the types in the test class
There was a problem hiding this comment.
The ones which test AssemblyRouteSource and NamespaceRouteSource specifically don't/can't. Those types are not passed through type scanning.
The bug only appeared if sagas are enabled and conventions is asked if a ref struct is a message type so it would require a very specific combination of things for this to pollute another test. There are many other types which would also be pulled in.
andreasohlund
requested review from
poornimanayar,
mikeminutillo and
lailabougria
| @@ -104,6 +104,13 @@ static bool IsCompatible(Type t, Type source) | |||
|
|
|||
| static bool IsTypeATimeoutHandledByAnySaga(Type type, IEnumerable<Type> sagas) | |||
| { | |||
| // MakeGenericType() throws an exception if passed a ref struct type | |||
| // Messages cannot be ref struct types | |||
There was a problem hiding this comment.
this is not the correct fix. types that do not match the convention should never get to this point. the bug is that RouteToEndpoint has a side effect of causing non message types to be treated as message types
There was a problem hiding this comment.
the bug is that RouteToEndpoint has a side effect of causing non message types to be treated as message types
In order to determine if any given type is a message type or not, we have to ask the conventions subsystem. Here is where that is happening.
If you have a saga in your endpoint, the Sagas feature adds a message convention to say that anything handled by a saga IS a message type, even if it matches no other conventions. This is how you can have timeout messages that do not conform to any registered convention.
To make that determination we use the type in question to create the two handler type interfaces that sagas might implement (IHandleMessages<> and IHandleTimeouts<>) and then check to see if any sagas implement those.
So unfortunately we cannot say that this type is not a message type without executing this code. We could (and possibly will) move the check even earlier in the conventions pipeline but this change is the least invasive one we could devise for a patch release.
There was a problem hiding this comment.
@mikeminutillo ahh. fair enough. thanks for the detailed explanation
| // Even though it is not a message type, it is still checked by passing it to conventions. | ||
| // The conventions added by Sagas were throwing an exception when passed a ref struct. | ||
| // See https://github.com/Particular/NServiceBus/issues/7179 for details. | ||
| ref struct RefStruct { } |
There was a problem hiding this comment.
The ones which test AssemblyRouteSource and NamespaceRouteSource specifically don't/can't. Those types are not passed through type scanning.
The bug only appeared if sagas are enabled and conventions is asked if a ref struct is a message type so it would require a very specific combination of things for this to pollute another test. There are many other types which would also be pulled in.
Fixes #7179