[release/7.0] Fix GUID interop in distributed transactions and block ARM #74570
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@jeffschwMSFT do you approve of this backport? |
|
@AaronRobinsonMSFT unless you object, we can hold off on this PR, as the arm64 issue still occurs (#74226 (comment)). |
|
Let me get a sign off going. |
Right. That is going to get some pushback. We should really root cause this and create a single PR for the full fix since there seems to be something else also going on here. |
|
Makes sense, |
AaronRobinsonMSFT
added
the
NO-MERGE
|
Just checking in, where are we likely to land with this one and who is holding the baton? As you know, the bar goes up to servicing level next week. |
|
@danmoseley this PR, as-is, apparently doesn't fix the original issue (intermittent AccessViolationException on arm64 only), see #74170 (comment) for some context. We're basically blocking on access to an arm64 machine which would allow us to further investigate and hopefully fix this; note that this is potentially a wider arm64 interop (or possibly MSDTC on arm64) issue. Unless @kunalspathak or someone else can help us out with this, I plan to submit a separate PR to disable distributed transactions entirely on arm64 (throw PlatformNotSupportedException). I'll probably be doing that early next week. |
|
@roji have you been able to reproduce the failure with this PR (and with the tests re-enabled)? I'm running the OleTxTests tests in a loop until I get a failure, but if you can get a dump, that would be preferable. |
|
@jkoritzinsky as far as I know, the AccessViolationException this tracks has only ever occured on amd64, which I don't have access to; are you saying you're seeing it (on non-arm64? on arm64?). If you're seeing this on arm64 and can help out, then see comment #74170 (comment); @AaronRobinsonMSFT proposed we experiment with manual COM marshalling, which I've pushed in #74226, and we indeed need a dump of the interop stub (but are blocked because of arm64 access). |
|
@roji there should be some arm64 machines available for investigations -- |
I have sent email internally to provide you with an access to the Arm64 machine. |
|
I was running it in an ARM64 machine. I let it run overnight, so I’ll see if I got a failure when I get to my machine today. |
|
My terminal got killed, so I'll try it again. |
|
@roji I've been running the Update: I'm at over 1000 executions without failure. Are we sure this still fails in a non-stress mode? |
|
At execution 1036, I recieved the following error (not an AV): |
|
FYI there are merge conflicts with the base branch now. |
|
It doesn't help really, but I did confirm from looking at the xunit code, that "The following constructor parameters did not have matching fixture data: OleTxFixture fixture" is just an outcome of failing to construct OleTxFixture due to the exception above. @roji either some fragility in the product, in the DTC or whatever below that, or in OleTxFixture,.. ? |
|
First, thanks everybody for helping out with this... Since we're about to branch for rc2 and we still have an unknown AccessViolationException on arm64, and since @AaronRobinson is OOF, I'm proposing to add a check to throw PlatformNotSupportedException if distributed transactions are attempted on arm64 (and also merge the already-approved GUID interop changes). We can continue investigating this post-rc2 (to at least exclude a deeper bug) or for 8.0, let's continue conversation on this in #74170.
|
roji
changed the title
|
Re log full - it might possibly be worth temporarily adding a throw in the finalizer of internaltransaction (can't see the type name on my phone right now) to verify it's not running. It's a long shot but if it was doing cleanup instead of dispose, and the tests exited before it ran, perhaps that could leave state in this "log". |
Is this something @roji should do before getting approval/merging for 7.0? Or something to follow up on in main? |
No, I don't think so. It's only showing on stress on Jeremy's machine so far, it's not clear whether it's product or not, and there's also no suggestion (I think) that this introduces it. Leaving that aside -- if I understand correctly, we believe that this feature is shippable for x64, with none of these issues seen there? Are you comfortable shipping the feature disabled for 32 bit and for Arm? Is x64 (Windows of course) where customers are asking for it? Is there a possibility of surprise if eg., they develop on x64 and attempt to deploy on Arm64 (at least it will fail immediately)? I don't have an opinion, what do you think? If we fix the AV and verify on Arm64, it does seem like we can enable it there in servicing. |
|
@danmoseley I think it reasonable to bring it to x64 only in .NET 7. There will only be a small subsection of customers using this anyway, any the primary target audience is existing server apps running on Windows .NET Framework, which will generally be x64. I imagine we will want to get ARM64 working for the future, but I don't think we can realistically do that in .NET 7, and, given the target audience, I don't think backing out the entire feature is a better approach. For x86, we can probably wait for feedback. I doubt we can do ARM64 in servicing, but we could consider that with a very strong case if the investigation shows conclusively that the broken part is simple and is safe to change. |
|
x64 only sounds good to me then. I do agree though we should continue investigation on Arm (eg it could be an issue common to both that only shows up there) |
|
@danmoseley thanks for helping out here - I think @ajcvickers summed it up above; we expect the vast majority of the users of this feature to only need x64. I do hope we'll get to the bottom of the ARM64 issue soon; we can then evaluate whether it makes sense to to unblock that in servicing. I think this PR still requires your approval (or tactics)? |
|
The goal for Windows ARM64 is to have a full parity with Windows x64 so that there are no blockers for customers who want to migrate to Windows ARM64. This is going against that goal. |
| case Architecture.X86: | ||
| throw new PlatformNotSupportedException(SR.DistributedNotSupportedOn32Bits); | ||
|
|
||
| case Architecture.Armv6: // #74170 |
There was a problem hiding this comment.
case Architecture.Armv6 is unnecessary. Armv6 is Unix-only platform.
Everybody (.NET, Windows, ...) is still working on long-tail of Windows Arm64 bugs, like this one. The fact that these bugs exist is not a good reason to completely disabling features. |
|
Has anyone here been able to reproduce the AV with the changes in the PR and with the tests re-enabled? |
To be sure we're on the same page, @jkotas are you saying we should not block distributed transactions on arm64 although we know it produces intermittent AccessViolationException? I thought this wouldn't be ideal from a reliability standpoint (and this is something we do intend to get to the bottom of). If this is what we want to do, I'll remove the exception from this PR and leave only the GUID interop fixes. |
|
Right, I do not think we should be blocking the feature on arm64 due to intermittent AccessViolationException. We should keep working on root-causing the intermittent AccessViolationException. (It is ok if the fix does not make it for .NET 7 GA and needs to wait for servicing. I guess there is also a possibility that the crash needs to be fixed in Windows.) |
|
Just curious, what's the blocker on x86? |
|
@danmoseley Can we get a decision here, or does this need to go to Tactics? Specifically, we would like to get this merged for rc2 because it contains other fixes, and we can do that either with or without blocking arm64. |
|
@danmoseley on x86 I get an ArgumentException when invoking DtcGetTransactionManagerExW (docs) (full exception details below). This is 100% consistent (no flakiness) and easy to reproduce. Full exception details |
carlossanlop
added
Servicing-consider
|
FYI 7.0 backport PRs do not need to go through Tactics yet until after the snap. But if it's critical to discuss in Tactics, I guess it's ok. |
rbhanda
added
Servicing-approved
|
@carlossanlop @jkotas my understanding from Tactics is that we want to merge this as-is for now (including the arm64 block), and possibly review later for removing the arm64 block before GA. So @carlossanlop you can go ahead and merge. |
carlossanlop
removed
the
NO-MERGE
|
Sure @roji. Removing the "no merge" label. |
ghost
locked as resolved and limited conversation to collaborators
Backport of #74226 to release/7.0
Fixes #74170
/cc @roji
Customer Impact
Hard crash in GUID interop when using new distributed transactions feature in .NET 7.
Also blocks ARM64 since it's not working.
Testing
Significant testing done on main PR.
Risk
Risk is small; fix clear bug in new feature.
IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.