-
Notifications
You must be signed in to change notification settings - Fork 649
Adds settings-only option to use Outbox in SendsAtomicWithReceiveMode #7486
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
base: release-9.2
Are you sure you want to change the base?
Conversation
src/NServiceBus.Core/Reliability/Outbox/SendSetAsDispatchedMessageBehavior.cs
Show resolved
Hide resolved
danielmarbach
left a comment
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 have a few concerns that I think we have to discuss
- How does that affect the transactional session? The transactional session already uses a similar dispatch message approach and I wonder if those cases should be aligned or uniformed somehow in case we are doing this change. It might allow us to make tradeoffs in the transactional session to directly dispatch the transport messages but use the mechanism here to set it as dispatched or some other fancy clever combination I cannot come up right now by only spending a few minutes looking at the code
- Another thing that I find concerning is that today I think we have designed and implemented quite a few assumptions around the interaction with the persister that also the transactional session leverages. Some of the persisters could technically rely on the context being passed to
Getalso being passed toSetAsDispatchedto achieve, for example, concurrency control by storing information in the context bag. This information would get lost and can lead to all sorts of issues. - If we further think about the transactional session use cases, we might also consider the local processing vs remote processing of the "control message" and then maybe the hard coded assumption of SendLocal might not be a good fit.
src/NServiceBus.Core/Reliability/Outbox/SendSetAsDispatchedMessageBehavior.cs
Outdated
Show resolved
Hide resolved
| return next(context); | ||
| } | ||
|
|
||
| return outboxStorage.SetAsDispatched(messageId, new ContextBag(), context.CancellationToken); |
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 for consistency it should be using the outbox seam and pass context.Extensions as the bag.
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.
We can't use IOutboxSeam here because for the SAWR mode it _noopstheSetAsDispatched. My idea for introducing IOutboxSeamwas that theTransportReceiveToBlaBlaBehaviorhas no extensibility points to tweak what is executed andIOutboxSeam` provides that.
So IOutboxSeam is a contract between that behavior and the Outbox feature. Within the Outbox feature (like in SetAsDispatched behavior), we interact with IOutboxStore directly.
My mental model of the transactional session is based on the split between sync part (while handling the web request) and async part (when the control message arrives), connected by that control message. If I understand correctly the current behavior is that the This is a good question but I don't see for now much opportunity for alignment. The significant difference is that for the I think that the we can look at this space like this. In order to implement atomic send and update the sends need to be stored and there needs to be a specific sequence of steps executed:
Compared to the
I think that the local processing does not apply to SAWR outbox because by definition it requires an incoming message. This means that we can safely assume that the control message that cleans the outbox can be sent to the local queue. |
|
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. |
Option requires explicit opt-in via settings
In
SendsAtomicWithReceiveModemode:Dispatchedimmediately after dispatching the outgoing operations (because the receive transaction can still roll back so they are not really dispatched)DispatchedAdded 4 tests to verify the behavior
Happy path (that a message is delivered)
That when a failure happens just before ACKing the received message, the outgoing messages are not released (and are indeed part of the receive transaction)
That after such a failure ☝️ a retry pushes out the messages
That the self-sent control message indeed marks the record as
Dispatched, preventing duplicate amplificationAdded strict sequential mode to AT transport in order to make sure the tests verify what they are supposed to. This mode consists of
LimitMessageProcessingConcurrencyTo(1);andb.ConfigureTransport<AcceptanceTestingTransport>().FifoMode = true;