-
Notifications
You must be signed in to change notification settings - Fork 648
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
Fix ref struct scan for sagas #7187
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
namespace NServiceBus.AcceptanceTests.Core.Conventions; | ||
|
||
using System; | ||
using System.Threading.Tasks; | ||
using AcceptanceTesting; | ||
using EndpointTemplates; | ||
using NServiceBus.AcceptanceTesting.Customization; | ||
using NUnit.Framework; | ||
|
||
public class When_scanning_an_assembly_containing_a_ref_struct_and_sagas_enabled : NServiceBusAcceptanceTest | ||
{ | ||
[Test] | ||
public void It_should_not_throw_an_exception() | ||
=> Assert.DoesNotThrowAsync( | ||
() => Scenario.Define<ScenarioContext>() | ||
.WithEndpoint<EndpointWithASaga>() | ||
.Run() | ||
); | ||
|
||
public class SomeMessage : IMessage | ||
{ | ||
public Guid BusinessId { get; set; } | ||
} | ||
|
||
// HINT: This will get picked up by the AssemblyRouteSource created by the routing call below | ||
// 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 { } | ||
|
||
class EndpointWithASaga : EndpointConfigurationBuilder | ||
{ | ||
public EndpointWithASaga() => EndpointSetup<DefaultServer>(cfg => cfg | ||
.ConfigureRouting() | ||
.RouteToEndpoint( | ||
typeof(RefStruct).Assembly, | ||
Conventions.EndpointNamingConvention(typeof(EndpointWithASaga)) | ||
) | ||
); | ||
|
||
|
||
class RealSagaToSetUpConventions : Saga<RealSagaToSetUpConventions.RealSagaToSetUpConventionsSagaData>, IAmStartedByMessages<SomeMessage> | ||
{ | ||
public Task Handle(SomeMessage message, IMessageHandlerContext context) => Task.CompletedTask; | ||
protected override void ConfigureHowToFindSaga(SagaPropertyMapper<RealSagaToSetUpConventionsSagaData> mapper) | ||
=> mapper.MapSaga(saga => saga.BusinessId).ToMessage<SomeMessage>(msg => msg.BusinessId); | ||
|
||
public class RealSagaToSetUpConventionsSagaData : ContainSagaData | ||
{ | ||
public virtual Guid BusinessId { get; set; } | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 ( 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mikeminutillo ahh. fair enough. thanks for the detailed explanation |
||
if (type.IsByRefLike) | ||
{ | ||
return false; | ||
} | ||
|
||
var timeoutHandler = typeof(IHandleTimeouts<>).MakeGenericType(type); | ||
var messageHandler = typeof(IHandleMessages<>).MakeGenericType(type); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ones which test
AssemblyRouteSource
andNamespaceRouteSource
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.