Particular / NServiceBus.AzureFunctions.InProcess.ServiceBus

Process messages in AzureFunctions using the Azure Service Bus trigger and the NServiceBus message pipeline.
Other
10 stars 5 forks source link

Properly Stop registered IEndpointInstance #296

Open codewisdom opened 3 years ago

codewisdom commented 3 years ago

We find the suggested approach in the documentation of setting up routing when dispatching messages outside of a handler to be cumbersome:

https://docs.particular.net/nservicebus/hosting/azure-functions-service-bus/#basic-usage-dispatching-outside-a-message-handler

Therefore, we spin up a IEndpointInstance as a singleton using the built in DI IFunctionsHostBuilder, providing all the routing config at startup. We know it's a best practice to call Stop() when spinning down, but we are not sure what (if any) the best practice would be in this Azure Functions context. Any suggestions would be appreciated.

SeanFeldman commented 3 years ago

We find the suggested approach in the documentation of setting up routing when dispatching messages outside of a handler to be cumbersome:

I assume you're referring to these lines:

var sendOptions = new SendOptions();
sendOptions.RouteToThisEndpoint();

That's not a recommendation, just a code sample, as the snippet cannot assume the injected IFunctionEndpoint has the routing configured or not.

Therefore, we spin up a IEndpointInstance as a singleton using the built in DI IFunctionsHostBuilder, providing all the routing config at startup

That's likely the same as what the documentation is showing here. Within builder.UseNServiceBus() you can configure the endpoint, including the routing.

We know it's a best practice to call Stop() when spinning down, but we are not sure what (if any) the best practice would be in this Azure Functions context. Any suggestions would be appreciated.

You don't need to stop anything as you do not control the receiving aspect or the moment when the function is taken down. A completed function execution does not terminate the function host. It lives there for a few mins, in anticipation, more invocations will take place. Leave it with Functions. I hope that helps.

codewisdom commented 3 years ago

Thanks @SeanFeldman. Much appreicated.

We are calling this in Startup.Configure:

private static void RegisterNServiceBusEndpoints(IFunctionsHostBuilder builder)
{
    var config = builder.GetContext().Configuration;

    builder.UseNServiceBus(() =>
    {
        var endpointConfiguration = new ServiceBusTriggeredEndpointConfiguration(
            Constants.testManagerEndpointName,
            Configuration.AzureWebJobsServiceBus);
        endpointConfiguration.AdvancedConfiguration.SendFailedMessagesTo("error");
        endpointConfiguration.AdvancedConfiguration.AuditProcessedMessagesTo("audit");
        endpointConfiguration.AdvancedConfiguration.DisableFeature<AutoSubscribe>();

        return endpointConfiguration;
    });

    var testManagerEndpointConfig =
        BuildtestEndpointConfiguration(config.GetRequired(Configuration.AzureWebJobsServiceBus));

    builder.Services.AddSingleton(typeof(IEndpointInstance),
        sp => Endpoint.Start(testManagerEndpointConfig).GetAwaiter().GetResult());
}

To be entirely clear, we are attempting to avoid sending the ExecutionContext when we need to send commands. Should we just pass null if we don't have it?

SeanFeldman commented 3 years ago

To be entirely clear, we are attempting to avoid sending the ExecutionContext when we need to send commands. Should we just pass null if we don't have it? What is it that you trying to avoid or address? Perhaps you should also share your trigger code for the completeness.

I'm confused. ExecutionContext is provided by Functions to your function method at run-time. What's the problem of passing it on?

And then this will not be needed, considering that it shouldn't be done like that at all.

   builder.Services.AddSingleton(typeof(IEndpointInstance),
       sp => Endpoint.Start(testManagerEndpointConfig).GetAwaiter().GetResult());

as

builder.UseNServiceBus(() =>);

already registered the endpoint which is available as long as the function app is live and is not scoped to a single triggered execution.

codewisdom commented 3 years ago

@SeanFeldman You have every right to be confused. We are new to Azure Functions and we've probably done a few things atypically. I don't think there is any issue with adding the ExecutionContext back, so that's probably what we'll do. Please keep this open a bit longer. Someone from this team will followup early next week, probably me.

SeanFeldman commented 3 years ago

@codewisdom, no worries. I would suggest to follow the documentation, register NServiceBus, inject IFunctionEndpoint and avoid IEndpointInstance.

codewisdom commented 3 years ago

Hi @SeanFeldman,

We are curious how the ExecutionContext is used inside the FunctionEndpoint. I took a look at the code and cannot find any usages of it. If we pass null, do we lose something? As far as I can tell, there are no guards checking for that.

Regards, Sam

SeanFeldman commented 3 years ago

Looks like it's ignored at this point. /cc @timbussmann

timbussmann commented 3 years ago

it is required when initiating the endpoint initially, for subsequent usage it's not strictly required but I'd not recommend call it with null. That seems to be an indication you're using it incorrectly.

Can you share some more details about why the approach outlined in the samples does not work for you @codewisdom?

codewisdom commented 2 years ago

Hi @timbussmann,

I confess to being a little confused. The code clearly shows that it is not used. I can point you to the exact lines if you need that.

We are trying to keep our services decoupled from the hosting platform, i.e. Azure Functions. I'm already confident we can get away with passing null there, so that's what we are going to do for now. We'll definitely keep an eye out for changes.

Thanks everyone.

Regards, Sam

timbussmann commented 2 years ago

I confess to being a little confused. The code clearly shows that it is not used. I can point you to the exact lines if you need that.

I think it might help if you could show what exactly you mean. It might make sense to share your whole solution with us to get a better look, you can just open a support case with us at our support page and submit your code there to keep it confidential if necessary.

We are trying to keep our services decoupled from the hosting platform, i.e. Azure Functions.

tbh that's what our package is supposed to do. You can focus on your business logic by writing message handlers that are compatible with any supported hosting platform/model while easily integrating with azure functions through this package. I'd generally not recommend to further abstract away from that given the rest of your solution should be easily portable already.

codewisdom commented 2 years ago

@timbussmann I may be missing something. Forgive me if so.

Here is the creation of the endPointFactory Func. As you can see, the parameter passed is not used.

https://github.com/Particular/NServiceBus.AzureFunctions.InProcess.ServiceBus/blob/228ca96a2119e3c6d411da6cc9003062091e48ae/src/NServiceBus.AzureFunctions.InProcess.ServiceBus/FunctionEndpoint.cs#L25

It is invoked in one place:

https://github.com/Particular/NServiceBus.AzureFunctions.InProcess.ServiceBus/blob/228ca96a2119e3c6d411da6cc9003062091e48ae/src/NServiceBus.AzureFunctions.InProcess.ServiceBus/FunctionEndpoint.cs#L144

SeanFeldman commented 2 years ago

FunctionExecutionContext was supposed to capture and provide access to the ILogger and Functions ExecutionContext but that has never materialized. https://github.com/Particular/NServiceBus.AzureFunctions.InProcess.ServiceBus/blob/228ca96a2119e3c6d411da6cc9003062091e48ae/src/NServiceBus.AzureFunctions.InProcess.ServiceBus/FunctionExecutionContext.cs#L14-L18

ILogger is passed from the function that is invoked and is optional. ExecutionContext was originally used to determine code locations but now is replaced with IFunctionsHostBuilder.GetContext() and likely could be removed. https://github.com/Particular/NServiceBus.AzureFunctions.InProcess.ServiceBus/blob/228ca96a2119e3c6d411da6cc9003062091e48ae/src/NServiceBus.AzureFunctions.InProcess.ServiceBus/FunctionsHostBuilderExtensions.cs#L74

timbussmann commented 2 years ago

I think I'm a bit lost about what we're exactly discussing here. The dependency on the ExecutionContext shouldn't be an issue as the package is handling all of that for you. If you're trying to call these methods yourself, I'd like to focus on that point because that sounds off to me. e.g. you mention in some earlier comment that you're having this code: builder.Services.AddSingleton(typeof(IEndpointInstance), sp => Endpoint.Start(testManagerEndpointConfig).GetAwaiter().GetResult()); which should definitely not be done. Can you share some more insights or maybe a small sample on what you're trying to achieve what the package currently can't support?

codewisdom commented 2 years ago

@timbussmann I'm not doing that anymore. We are passing null when calling Send and Publish. It is clearly not used. We wrapped all that in a service to make it simple. I think we can close this out. That being said, I think it's pretty bad that the public method expects something that is simply not used. Not sure how to fix without a breaking change. Guess you'll need to figure that out.

timbussmann commented 2 years ago

We are passing null when calling Send and Publish. It is clearly not used. We wrapped all that in a service to make it simple.

does that mean you're calling this from a place where you don't have the context available? I'm really not sure whether I understand the full issue here, so some kind of demo/repro sample would be extremely helpful to get a better understanding of why this API is currently an issue for your project.

codewisdom commented 2 years ago

@timbussmann We are doing our best to make our services layer decoupled from the hosting platform, Azure Functions. We've wrapped the IFunctionEndpoint in a service that serves our purposes just fine. That way, we can swap out that service's guts if we ever needed to use a different transport. Does that make sense?

timbussmann commented 2 years ago

@codewisdom we usually recommend to not build further abstraction over the NServiceBus APIs as they are in itself already abstractions over supported transports, persistences, features etc.

So do I understand correctly that you'd like to change the current send APIs, e.g. public Task Send(object message, ExecutionContext executionContext, ILogger functionsLogger = null, CancellationToken cancellationToken = default) to public Task Send(object message, ILogger functionsLogger = null, CancellationToken cancellationToken = default) instead?