Skip to content

Conversation

@timbussmann
Copy link
Contributor

@timbussmann timbussmann commented Aug 3, 2021

Adding a new Process method to the IFunctionEndpoint interface while obsoleting the old process method. The new method which is intended to be called by users that do not use the auto-generated trigger will detect the AutoComplete setting by walking up the CallStack.

This comes at an additional computation & allocation cost but prevents users from using the wrong tx/non-tx method with incompatible AutoComplete settings.
The FunctionEndpoint class itself exposes the explicit operations to do transactional/non-transactional processing for advanced uses cases. This is also the API used by the generated triggers.

@timbussmann
Copy link
Contributor Author

a rough test with Benchmark.net indicates a ~30ms computation overhead and ~7KB extra allocations caused by the stackwalk logic per invocation.

@mikeminutillo
Copy link
Member

What if we put a version with a boolean parameter behind an .Advanced property. So you can either do:

functionEndpoint.Advanced.Process(message, executionContext, messageReceiver, autoComplete, logger) and we'll obey the autoComplete setting provided by the caller.

Or:

functionEndpoint.Process(message, executionContext, messageReceiver, logger) and we'll try to infer autoComplete from the attribute and use it to call the advanced version.

If we cannot find the attribute, the exception message can point the user to the advanced method. Our generated code can jump straight to using the advanced method. People who do not want the overhead caused by the trigger-finding code can fall back to using the advanced method. Users who are hand-generating the function are unlikely to call the advanced method.

I spiked what this would look like in #247

@timbussmann timbussmann marked this pull request as ready for review August 5, 2021 10:26
/// <summary>
/// Processes a message received from an AzureServiceBus trigger using the NServiceBus message pipeline.
/// </summary>
public Task ProcessNonTransactional(Message message, ExecutionContext executionContext, IMessageReceiver messageReceiver, ILogger functionsLogger = null) => Process(message, executionContext, functionsLogger);
Copy link
Member

Choose a reason for hiding this comment

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

Should we be calling in to an obsolete method? We're going to have to fix it as soon as we update to 2.0 anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was just to not duplicate the code because the obsoleted message still needs to work and it can't call ProcessNonTransactional because of the missing receiver (or pass null but I didn't like that). I'm fine either way.

Copy link
Member

@mikeminutillo mikeminutillo left a comment

Choose a reason for hiding this comment

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

Happy for this to go to the transactions branch

# Conflicts:
#	src/NServiceBus.AzureFunctions.InProcess.ServiceBus/FunctionEndpoint.cs
#	src/ServiceBus.Tests/FunctionEndpointTests.cs
@timbussmann timbussmann merged commit aeb893f into transactions Aug 6, 2021
@timbussmann timbussmann deleted the auto-detect branch August 6, 2021 11:32
timbussmann added a commit that referenced this pull request Aug 9, 2021
* first draft of transaction support

* add missing logging integration

* tweaks

* Add support for cross-entity transactions configuration via attribute

* Approve API

* switch invoked method based on transaction setting

* please code style

* make the generated code valid syntax

* update approval files

* verify generated trigger compiles without errors

* remove optional ctor parameter for public attribute properties

* update tests

* cleanup tests

* obsolete NServiceBusEndpointNameAttribute

* suppress CS0618

* use single process implementation

* delete comment

* Native message may not have a messageId

* Extract transaction strategy

* use single process implementation

* delete comment

* Native message may not have a messageId

* add some unit tests around the Process implementation

* Extract transaction strategy

* Fix tests

* Refactor

* Auto-detect AutoComplete setting (#244)

* locate attribute to read AutoComplete value

* rename public APIs

* remove obsolete method usage

* extract reflection code into helper

* add more details to the reflection helper exception message

* merge AttributeDiscoverer and ReflectionHelper methods

* Update src/NServiceBus.AzureFunctions.InProcess.ServiceBus/NServiceBusTriggerFunctionAttribute.cs

Co-authored-by: Mike Minutillo <mike.minutillo@particular.net>

* use explicit interface impl. to hide Process on FunctionEndpoint

* inline SafeCompleteAsync

* update attribute property name

* use existing function name

* approve API changes

* prevent potential NRE

Co-authored-by: Sean Feldman <feldman.sean@gmail.com>
Co-authored-by: Sean Feldman <SeanFeldman@users.noreply.github.com>
Co-authored-by: Mike Minutillo <mike.minutillo@particular.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants