Port Windows/MSDTC distributed transactions support #72051

roji · 2022-07-12T20:18:13Z

Following #71433, this PR brings over the .NET Framework distributed transaction porting the native shim to fully managed code.

There's some remaining work and cleanup but the bulk of the work should be done and ready for at least a first review. Would appreciate a good look as I'm a newcomer to porting C++ code to C#, COM interop etc.

The code supports a successful distributed transaction flow with SqlClient, tested via a console program, but has no proper test coverage yet. I'll be looking at the old netfx tests and adding test coverage next.

/cc @stephentoub @AaronRobinsonMSFT @jkotas @MichalStrehovsky @vitek-karas @ajcvickers

Closes #715

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/DtcInterfaces.cs

src/libraries/System.Transactions.Local/src/ILLink/ILLink.Descriptors.LibraryBuild.xml

roji · 2022-07-12T23:29:07Z

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/OletxTransaction.cs

+        }
+
+        // TODO: Do we want to keep supporting this?
+        protected OletxTransaction(SerializationInfo? serializationInfo, StreamingContext context)


Note this serialization constructor.

I'm not sure what the state of serialization is exactly - this doesn't use BinaryFormatter, just serializes as an opaque byte array.

/cc @stephentoub @GrabYourPitchforks

// TODO: Do we want to keep supporting this?

I think "no" :-) but @GrabYourPitchforks should make the call.

...raries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/CachedInterfaceBase.cs

stephentoub

Skimmed the beginnings of this and left a few nits (I realize you didn't write much of the code). I'll review the whole thing more when it's out of draft.

src/libraries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/CachedReceiver.cs

...ibraries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/CachedTransmitter.cs

...es/System.Transactions.Local/src/System/Transactions/DtcProxyShim/NotificationShimFactory.cs

...raries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/ResourceManagerShim.cs

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/DTCTransactionManager.cs

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/DtcInterfaces.cs

stephentoub · 2022-07-13T14:45:42Z

...braries/System.Transactions.Local/src/System/Transactions/Oletx/OletxDependentTransaction.cs

+
+namespace System.Transactions.Oletx
+{
+    [Serializable]


I expect we want to remove all such attribution.
cc: @GrabYourPitchforks

See #72051 (comment).

AaronRobinsonMSFT

I've commented on the COM interfaces. I'll review all when it is out of draft.

AaronRobinsonMSFT · 2022-07-13T18:28:50Z

...System.Transactions.Local/src/System/Transactions/DtcProxyShim/DTCInterfaces/IPrepareInfo.cs

+    /// <param name="pPrepInfo">
+    /// Pointer to the caller allocated buffer to receive the prepare information. The size of pPrepInfo is determined by calling <see cref="GetPrepareInfoSize" />.
+    /// </param>
+    void GetPrepareInfo([MarshalAs(UnmanagedType.LPArray)] byte[] pPrepInfo);


I believe you also want [Out] here.

Thanks, I'm doing the same for ITransactionImportWhereabouts.GetWhereabouts, ITransactionExport.GetTransactionCookie, ITransactionReceiver.MarshalReturnToken, ITransactionTransmitter.MarshalPropagationToken.

Makes sense?

...em.Transactions.Local/src/System/Transactions/DtcProxyShim/DTCInterfaces/IResourceManager.cs

...System.Transactions.Local/src/System/Transactions/DtcProxyShim/DTCInterfaces/IPrepareInfo.cs

...em.Transactions.Local/src/System/Transactions/DtcProxyShim/DTCInterfaces/IResourceManager.cs

.../System.Transactions.Local/src/System/Transactions/DtcProxyShim/DTCInterfaces/ITmNodeName.cs

...sactions.Local/src/System/Transactions/DtcProxyShim/DTCInterfaces/ITransactionTransmitter.cs

AaronRobinsonMSFT · 2022-07-13T18:50:10Z

...ns.Local/src/System/Transactions/DtcProxyShim/DTCInterfaces/ITransactionVoterNotifyAsync2.cs

+/// See https://docs.microsoft.com/en-us/previous-versions/windows/desktop/ms678930(v=vs.85).
+/// </remarks>
+[ComImport, Guid("5433376B-414D-11d3-B206-00C04FC2F3EF"), InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
+internal interface ITransactionVoterNotifyAsync2


This inherits from ITransactionOutcomeEvents in the COM declaration. This needs at least 4 placeholders to fix the vtable layout.

Thanks; I've copied the 4 methods from ITransactionOutcomeEvents to ITransactionVoterNotifyAsync2, before it's added method (VoteRequest). Can you confirm that's the proper way to do it (rather than e.g. having ITransactionVoterNotifyAsync2 extend ITransactionOutcomeEvents)?

roji · 2022-07-19T14:15:44Z

This should be ready for review. I'll continue working on test coverage in the coming days, but the code should be more or less ready.

...libraries/System.Transactions.Local/src/System/Transactions/Oletx/OletxTransactionManager.cs

GrabYourPitchforks · 2022-07-25T19:19:55Z

Re: serialization, since you removed the formatter calls within Reenlist and ConvertToByteArray you may as well also remove the [Serializable] attribute and ISerializable interfaces, plus all GetObjectData implementations, unless they're needed as part of the public API surface.

I also see some stray references to the System.Security.Permissions namespace. Those APIs are all obsolete, so be sure to remove those references.

roji · 2022-07-26T14:52:14Z

@GrabYourPitchforks

I also see some stray references to the System.Security.Permissions namespace. Those APIs are all obsolete, so be sure to remove those references.

Thanks, removed the last lingering one.

Re: serialization, since you removed the formatter calls within Reenlist and ConvertToByteArray you may as well also remove the [Serializable] attribute and ISerializable interfaces, plus all GetObjectData implementations, unless they're needed as part of the public API surface.

Here's a bit more context on this... There are two places where serialization/binary formatting happens in this library:

When handing a user an opaque byte[] as a recovery information, exposed to the resource manager via PreparingEnlistment.RecoveryInformation. The resource manager (e.g. database) is supposed to persist that information, and if a crash occurred, call Reenlist, passing it back the same opaque bit of information.
- The recovery information is obtained from MSDTC via COM (in EnlistmentNotifyShim), where it's already an opaque binary blob. For some unclear reason, the previous code passed this through BinaryFormatter (in ConvertToByteArray) to get another (formatter) opaque blob, and returned that. The corresponding unwrapping via BinaryFormatter happened in Reenlist.
- All I did here was remove the additional (useless) wrapping via BinaryFormatter. Beyond that we must keep this, since it's part of the necessary 2PC recovery flow (and shouldn't be problematic).
OletxTransaction is serializable
- This allows propagating a transaction to another process/machine, which is something that happens in distributed scenarios.
- Serialization is implemented by calling into MSDTC via COM, obtaining a "propagation token" (via TransactionInterop.GetTransmitterPropagationToken(), here), which is an opaque token, and returning that as the serialized representation.
- No BinaryFormatter is or was involved here; at least in that sense it's safe.
- If we remove this, users can still propagate transactions by calling TransactionInterop.GetTransmitterPropagationToken themselves, but it would add friction and (needlessly?) break code using Sys.Tx.

Hopefully that provides context. As the PR stands, there's no longer any BinaryFormatter anywhere, and OletxTransaction serialization seems safe. Let me know what you think.

stephentoub

That's a lot of code :) I skimmed through it and left a few very minor nits, but my assumption is that almost all of the code came over with little to no semantic or meaningful changes, yes? I agree with one of the comments you left about not doing work to optimize. I also see you have tests; do they provide enough coverage?

...System.Transactions.Local/src/System/Transactions/DtcProxyShim/DTCInterfaces/IPrepareInfo.cs

...raries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/DtcProxyShimFactory.cs

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/OletxEnlistment.cs

...libraries/System.Transactions.Local/src/System/Transactions/Oletx/OletxTransactionManager.cs

roji · 2022-08-04T14:37:07Z

@stephentoub sorry for the big pause, was busy with some other urgent stuff - will now work to merge this ASAP.

That's a lot of code :) I skimmed through it and left a few very minor nits, but my assumption is that almost all of the code came over with little to no semantic or meaningful changes, yes? I agree with one of the comments you left about not doing work to optimize.

Yes, that's true - I've done my best to change absolutely nothing and simply convert C++ to C# here, only doing meaningful changes where absolutely necessary (in rare cases). I generally doubt this code is used in a perf-sensitive way which would justify the risk of optimizing/refactoring, at least not at this point...

I also see you have tests; do they provide enough coverage?

Not enough... I've done various manual testing against SQL Server to confirm that things work, but automated testing support is lacking. Automated testing is difficult since it involves coordinating between multiple processes, crashing them at specific points, and for some (extreme0 edge cases, even doing stuff with the Windows MSDTC service. I'll be spending some time in the next few days to try to improve coverage at least to some extent - at the worst case maybe I'll continue working on test coverage after merging for rc1...

How does that sound?

@GrabYourPitchforks @stephentoub can you take a look at #72051 (comment) and confirm whether you really want the serialization support removed?

roji · 2022-08-05T13:02:25Z

@stephentoub @AaronRobinsonMSFT do you guys have any idea about the build failures? There's various "Managed parameter or return types require runtime marshalling to be enabled" errors reported on other projects, even though they're obviously related to my stuff. I've seen this page but I'm not sure what's the right thing to do here.

src/libraries/System.Transactions.Local/src/System.Transactions.Local.csproj

...System.Transactions.Local/src/System/Transactions/DtcProxyShim/DTCInterfaces/IPrepareInfo.cs

danmoseley · 2022-08-10T21:54:17Z

@roji Is this destined for .NET 7? The cutoff is almost upon us so wondering who owns next action.

roji · 2022-08-10T22:39:57Z

@danmoseley yes, I'm planning to merge this for 7.0. I'm currently blocking on flaky CI test failures, and will spend some time in the next couple of days trying to find the cause....

Closes dotnet#715

roji · 2022-08-12T09:58:16Z

The remaining distributed transaction test failures occur only on 32-bit, these are consistent and easy to reproduce locally as well. As discussed offline, I've skipped the MSDTC tests on 32-bit for merging for rc1, I'll look into this soon.

julielerman · 2022-08-12T13:31:56Z

bravo, shay!

roji · 2022-08-12T16:01:02Z

Thanks @julielerman :)

ghost assigned roji Jul 12, 2022

ghost added the area-System.Transactions label Jul 12, 2022

roji commented Jul 12, 2022

View reviewed changes

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/DtcInterfaces.cs Outdated Show resolved Hide resolved

roji commented Jul 12, 2022

View reviewed changes

src/libraries/System.Transactions.Local/src/ILLink/ILLink.Descriptors.LibraryBuild.xml Outdated Show resolved Hide resolved

This was referenced Jul 12, 2022

Add necessary trimmer files for not trimming COM types. roji/runtime#1

Closed

Bring over distributed transactions support for Windows #71433

Closed

roji commented Jul 12, 2022

View reviewed changes

stephentoub reviewed Jul 13, 2022

View reviewed changes

...raries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/CachedInterfaceBase.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jul 13, 2022

View reviewed changes

AaronRobinsonMSFT reviewed Jul 13, 2022

View reviewed changes

AaronRobinsonMSFT mentioned this pull request Jul 15, 2022

Permit repeating WaitHandle of infinite timeout #72267

Merged

roji marked this pull request as ready for review July 19, 2022 14:14

roji commented Jul 19, 2022

View reviewed changes

...libraries/System.Transactions.Local/src/System/Transactions/Oletx/OletxTransactionManager.cs Show resolved Hide resolved

roji force-pushed the DistributedTransactions2 branch from 623cf41 to 7767287 Compare July 26, 2022 14:59

stephentoub approved these changes Jul 26, 2022

View reviewed changes

roji force-pushed the DistributedTransactions2 branch from 7767287 to 699bdcb Compare August 4, 2022 14:44

jkotas reviewed Aug 5, 2022

View reviewed changes

src/libraries/System.Transactions.Local/src/System.Transactions.Local.csproj Outdated Show resolved Hide resolved

jkotas reviewed Aug 5, 2022

View reviewed changes

...System.Transactions.Local/src/System/Transactions/DtcProxyShim/DTCInterfaces/IPrepareInfo.cs Outdated Show resolved Hide resolved

roji force-pushed the DistributedTransactions2 branch from 64057d7 to c94e4ff Compare August 7, 2022 18:33

roji force-pushed the DistributedTransactions2 branch from 5f33b62 to a572b48 Compare August 11, 2022 22:22

Port distributed transaction support for Windows

b84598d

Closes dotnet#715

roji force-pushed the DistributedTransactions2 branch from a572b48 to b84598d Compare August 12, 2022 07:07

roji merged commit c55d76d into dotnet:main Aug 12, 2022

roji deleted the DistributedTransactions2 branch August 12, 2022 09:58

MichalStrehovsky mentioned this pull request Aug 12, 2022

[NativeAOT] Follow up changes for the suspension loop routine. #73741

Merged

kunalspathak mentioned this pull request Aug 18, 2022

Distributed transactions: AccessViolationException on arm64 #74170

Closed

radekdoulik mentioned this pull request Aug 24, 2022

[wasm] Size regression #74506

Closed

eerhardt mentioned this pull request Sep 2, 2022

Make System.Transactions.Local trimmable on Windows #75031

Closed

ghost locked as resolved and limited conversation to collaborators Sep 11, 2022

Port Windows/MSDTC distributed transactions support #72051

Port Windows/MSDTC distributed transactions support #72051

Uh oh!

Conversation

roji commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roji commented Jul 19, 2022

Uh oh!

Uh oh!

GrabYourPitchforks commented Jul 25, 2022

Uh oh!

roji commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roji commented Aug 4, 2022

Uh oh!

roji commented Aug 5, 2022

Uh oh!

Uh oh!

Uh oh!

danmoseley commented Aug 10, 2022

Uh oh!

roji commented Aug 10, 2022

Uh oh!

roji commented Jul 12, 2022 •

edited

Loading

roji commented Jul 26, 2022 •

edited

Loading

roji commented Aug 12, 2022 •

edited

Loading