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

Fix ref struct scan for sagas #7187

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
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()
);

// 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 { }
Copy link
Member Author

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

Copy link
Member

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 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.


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; }
}
}
}

public class SomeMessage : IMessage
{
public Guid BusinessId { get; set; }
}
}
7 changes: 7 additions & 0 deletions src/NServiceBus.Core/Sagas/Sagas.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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);

Expand Down