Ports Fix deadlock in transaction (#1242) to .NET #2161
Conversation
|
Failures seems related to this change. |
Codecov ReportAttention:
... and 13 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
|
The test seems to fail only on a few targets. I enabled an existing test from the original PR to now run on NET too. |
|
@cheenamalhotra any ideas/pointers? |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionEnlistmentTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
danielmarbach
force-pushed
the
fix-deadlock
branch
from
52d24f3
to
0469097
Compare
|
One last problem to address, it seems
|
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
|
NET 7 and higher requires non-32 bit processes, see https://github.com/dotnet/runtime/blob/main/src/libraries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/DtcProxyShimFactory.cs#L69-L72. So I used the same check negated (although that is not entirely accurate it is a good enough start) |
|
@cheenamalhotra @JRahnama Fingers crossed this is ready to go now. I would appreciate a review |
| @@ -439,6 +439,17 @@ public static bool IsTargetReadyForAeWithKeyStore() | |||
| ; | |||
| } | |||
|
|
|||
| public static bool IsSupportingDistributedTransactions() | |||
There was a problem hiding this comment.
Decided to use fully qualified names here to avoid having to do if/def in the using section
| @@ -439,6 +439,17 @@ public static bool IsTargetReadyForAeWithKeyStore() | |||
| ; | |||
| } | |||
|
|
|||
| public static bool IsSupportingDistributedTransactions() | |||
There was a problem hiding this comment.
I tried to use the same Is pattern although I felt is is a bit of a clumsy name. I would have preferred SupportsDistributedTransactions but I wanted to make sure it fits the style of other such methods.
| #if NET7_0_OR_GREATER | ||
| return OperatingSystem.IsWindows() && System.Runtime.InteropServices.RuntimeInformation.ProcessArchitecture != System.Runtime.InteropServices.Architecture.X86 && IsNotAzureServer(); | ||
| #elif NETFRAMEWORK | ||
| return System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.Windows) && IsNotAzureServer(); |
There was a problem hiding this comment.
Nit: .NET Framework is not built/tested on non-windows, so platform check can be avoided.
There was a problem hiding this comment.
Isn't it safer to leave it as is? Happy to change it if you wish so
|
@cheenamalhotra It seems the enclaves tests are running into timeouts but when I look at the stack trace I do not see any of the code I touched being in the stack trace. Is this a red herring? |
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs
Outdated
Show resolved
Hide resolved
|
Is this something that should also be fixed in the SqlClient in the runtime repo? |
|
@cheenamalhotra Do you have any insights on #2161 (comment) and #2161 (comment)? Is there anything else you need from my end to push this forward? |
|
@danielmarbach Any updates? |
|
@Chucked why are you pinging me? I'm a community contributor who has gone through the process of fixing this bug but I'm at the mercy of people at Microsoft to merge this which hasn't happened yet nor I have a gotten inputs on the failing tests |
Got it. Thank you for the reply. I am just really interested in having these changes merged. |
|
Please bear with us as we’re working on some internal updates. We apologize for the delay and will get back to you as soon as possible. |
|
@David-Engel I see you've added this to the 5.2.0-preview4 milestone. Any sort of time frame for when 5.2.0 is going to be released? Given that this is a bug fix, is there any chance of it being backported and released sooner as a 5.1.2 release perhaps? |
|
We just set a new target of Dec 7 for 5.2 preview 4. We have some internal fires that have pushed things back and there are still a number of PRs we want to get into a preview before GA. That means 5.2 GA won't be until Q1 2024, at the soonest. I'd prefer to see this fix get some time under a preview release before going into a hotfix. So it's not going into 5.1.2. But it will likely go into a future hotfix. |
If this was a new fix, I could understand this reasoning. However, this fix is already live in 5.1.1 for the Framework side of things, but it was missed for the Core side. This bug is actively blocking us and prevents DTC use on .NET, which has been supported on Windows since the release of .NET 7. Given all of this, any chance of you reconsidering shipping this fix sooner? |
big +1 for this. would really like to see this in a patch ASAP |
|
We have this currently targeting a 5.1 hotfix in early January. |
…ction against .NET 7 Backport [GH PR dotnet#2161](dotnet#1242) - Fix deadlock in transaction against .NET 7 Related work items: dotnet#1242
|
FYI the fix is now in 5.1.4 |
.NET 7 and higher supports distributed transactions on Windows. For more details see dotnet/runtime#715.
Unfortunately, the code that is responsible for the transactions in the Netcore part of the SqlClient has not been adopted to address #1124 which was fixed with #1242. This PR brings over the changes from #1242 and makes sure the distributed transaction on .NET running on Windows are not deadlocking.
We ran into the same issue as described in #1124 when trying to enable distributed transaction support on NET8 on Windows.