Record transport transaction state explicitly instead of inferring it - #1756
Conversation
|
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. |
|
ping |
|
I pushed what I believe are simplifications and one commit that contains a proposal that would switch to record types that in the future we could then use with C sharp discriminated union support in C sharp 15 we could create a closed hierarchy. It has one downside, it creates one class allocation more compared to the state approach. But given the complexity of this connection handling and that it makes it more expressive, it might be worthwhile the trade-off. But that's why I called the commit discussion for the proposal, and if we decide, we can just drop it. Do you have any thoughts, @dvdstelt ? |
|
I like the records, mostly for the non-null connection/transaction per case rather than the future C# 15 exhaustiveness. But storing the state next to SqlConnection/SqlTransaction reintroduced the two-sources-of-truth problem your previous commit removed. So I pushed two commits on top:
If you're OK with this shape, let's merge. Although there's still no review. |
The dispatcher reverse-engineered the transaction mode by probing which entries happened to be present in the TransportTransaction, through an order-dependent if/else chain. Every TransportTransaction is created by code that already knows the mode, so stamp it explicitly and switch on it when dispatching. Inference remains only as a fallback for instances created outside the transport (core's empty one, hand-rolled ones). Also consolidates the ReceiveOnlyTransactionMode key, which was defined twice and only worked because both strings happened to match.
SendOptionsExtensions hand-assembled the TransportTransaction with its own copy of the key strings while PublishOptionsExtensions used the TransportTransactions factory. Use the factory in both places and drop the now-unused duplicate key from SettingsKeys.
…d connection management
Keeps the record hierarchy from the proposal but makes it a projection over the TransportTransaction entries rather than a second copy of them. Storing the state record under its own key meant the connection and native transaction were held twice: once inside the record and once under the well-known SqlConnection/SqlTransaction keys that SQL persistence and hand-rolled integrations write. The record won on the read path, so any later mutation of those entries would leave the dispatcher silently using a stale connection. Deriving on every read restores the single source of truth without giving up the pattern matching. OutsideHandler, ReceiveOnly and AmbientTransaction now carry no data, because the dispatcher opens its own connection for all three and relies on Transaction.Current for the ambient case. That drops the null-forgiving operators the ReceiveOnly branch needed and leaves only NoTransaction, SendsAtomicWithReceive and UserProvided allocating, so send-only and TransactionScope dispatches allocate nothing. The factories go back to writing only the storage entries, which also restores the original behaviour of throwing at dispatch rather than eagerly inside UseCustomSqlTransaction. MessageDispatcher is unchanged.
The mapping from stored entries to dispatch state had no unit tests at all. Only the integration tests exercised it, so catching a regression required a database. Covers the six states produced by the factories, the hand-rolled shapes that external integrations and the core create, the invalid combinations that throw, and that the state follows the entries when they change after creation.
a0f9fd6 to
6ee8670
Compare
2 participants
Why
I always had issues reading the many if-statements on what state/context we're in. So I created this and hope it's more readable from now on.
What
MessageDispatcher.DispatchDefaultreverse-engineered the transaction mode by probing which entries happened to be present in theTransportTransactionbag, through an order-dependent if/else chain of extension methods (OutsideOfHandler,IsNoTransaction,IsReceiveOnly,IsSendsAtomicWithReceive,IsTransactionScope). EveryTransportTransactionis created by code that already knows the mode, so this PR stamps aTransportTransactionStateinto the transaction at creation time and turns the dispatcher into a single exhaustive switch.TransportTransactionStateenum; allTransportTransactionsfactory methods record it.DispatchDefaultswitches on the state, with a comment per case explaining how sends relate to the receive transaction. The two identical "open a dedicated connection" blocks (outside handler and receive-only) collapse into one case.InferState) for instances not created by the transport: the empty one core creates for dispatches outside the message processing pipeline, and hand-rolled ones used by external integrations. It mirrors the old chain's semantics, including its ordering."SqlTransport.ReceiveOnlyTransactionMode"key was defined in two classes; the writer used one copy and the readers the other, working only because the strings happened to match. It now lives once inTransportTransactionKeysand is still written to the bag for downstream components (e.g. SQL persistence).SendOptionsExtensionshand-assembled user-provided transactions with its own copy of the key strings whilePublishOptionsExtensionsused theTransportTransactions.UserProvided(...)factory; both now use the factory, and the duplicate key was removed fromSettingsKeys.Behavior notes
No public API changes (API approval tests unchanged). One deliberate alignment: a user-provided connection-only transaction reaching default (non-immediate) dispatch previously got wrapped in a new transaction, while the same bag in isolated dispatch did not. That path is unreachable in practice because
UseCustomSqlConnection/UseCustomSqlTransactionforce immediate dispatch; both paths now behave identically (dispatch on the user's connection). Everything else is a faithful translation of the old chain.