SendsAtomicWithReceive support #68
Conversation
| /// Processes a message received from an AzureServiceBus trigger using the NServiceBus message pipeline. | ||
| /// </summary> | ||
| public async Task Process(Message message, ExecutionContext executionContext, ILogger functionsLogger = null) | ||
| public async Task Process(Message message, ExecutionContext executionContext, ILogger functionsLogger = null, IMessageReceiver messageReceiver = null) |
There was a problem hiding this comment.
I think we should consider creating a dedicated overload where the message receiver is not optional (and maybe even name it differently)?
There was a problem hiding this comment.
What benefits will it provide?
There was a problem hiding this comment.
better indication to the user what it does to provide this parameter
…he ASB transport)
Only 3 types should be at the NServiceBus scope: FunctionEndpoint, FunctionExecutionContext, and ServiceBusTriggeredEndpointConfiguration
SeanFeldman
force-pushed
the
sends-atomic-with-receive
branch
from
e274b65 to
2dda284
Compare
| functionExecutionContext.Logger.LogDebug($"Failed to move a poisonous message with native ID: `{message.MessageId}` to the dead-letter queue. Message will be retried.", ex); | ||
| } | ||
|
|
||
| return; |
There was a problem hiding this comment.
if we would rethrow here instead, what would happen given that we moved the message to the DLQ already?
| { | ||
| public static Task SafeCompleteAsync(this IMessageReceiver messageReceiver, TransportTransactionMode transportTransactionMode, string lockToken) | ||
| { | ||
| if (transportTransactionMode != TransportTransactionMode.None) |
There was a problem hiding this comment.
that will never be the case because we only switch between SendOnly and SendsAtomicWithReceive?
| { | ||
| if (transportTransactionMode != TransportTransactionMode.None) | ||
| { | ||
| return messageReceiver.CompleteAsync(lockToken); |
There was a problem hiding this comment.
wouldn't that currently throw NREs when not providing a message receiver? (same issue on all the extension methods here)
| await messageReceiver.SafeAbandonAsync(transportTransactionMode, lockToken).ConfigureAwait(false); | ||
|
|
||
| // Indicate to the Functions runtime not to complete the incoming message | ||
| throw; |
There was a problem hiding this comment.
this part should probably not be included in the try/catch because it would be just rethrown again on line 126 with an incorrect log message?
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I had a look through this and it looks like it's on the right track. We might want to consider dropping the transaction scope in favor of explicitly floating a commitable transaction. |
Have to, not might want 🙂 |
Fixes Particular/NServiceBus.AzureFunctions.Worker.ServiceBus#43
WARNING: experimental and needs to be validated