dotnet
Port Windows/MSDTC distributed transactions support
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
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.
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.
@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 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 https://github.com/dotnet/runtime/pull/72051#issuecomment-1195587152 and confirm whether you really want the serialization support removed?
@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.
@roji Is this destined for .NET 7? The cutoff is almost upon us so wondering who owns next action.
@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....
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.
