Uh oh!
There was an error while loading. Please reload this page.
[release/7.0] Fix GUID interop in distributed transactions and block ARM - #74570
Conversation
roji
commented
Aug 25, 2022
carlossanlop
commented
Aug 25, 2022
@jeffschwMSFT do you approve of this backport? |
roji
commented
Aug 25, 2022
@AaronRobinsonMSFT unless you object, we can hold off on this PR, as the arm64 issue still occurs (#74226 (comment)). |
AaronRobinsonMSFT
commented
Aug 25, 2022
Let me get a sign off going. |
AaronRobinsonMSFT
commented
Aug 25, 2022
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. |
roji
commented
Aug 25, 2022
Makes sense, |
danmoseley
commented
Sep 7, 2022
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. |
roji
commented
Sep 7, 2022
@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. |
jkoritzinsky
commented
Sep 7, 2022
@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. |
roji
commented
Sep 8, 2022
@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 -- |
kunalspathak
commented
Sep 8, 2022
I have sent email internally to provide you with an access to the Arm64 machine. |
jkoritzinsky
commented
Sep 8, 2022
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. |
jkoritzinsky
commented
Sep 8, 2022
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? |
jkoritzinsky
commented
Sep 8, 2022
At execution 1036, I recieved the following error (not an AV): |
carlossanlop
commented
Sep 8, 2022
FYI there are merge conflicts with the base branch now. |
danmoseley
commented
Sep 9, 2022
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.
|
d170a14 to
d5efe59Comparedanmoseley
commented
Sep 9, 2022
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". |
ajcvickers
commented
Sep 9, 2022
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. |
danmoseley
commented
Sep 9, 2022
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) |
roji
commented
Sep 11, 2022
@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)? |
jkotas
commented
Sep 11, 2022
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.
jkotas
commented
Sep 11, 2022
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. |
jkoritzinsky
commented
Sep 11, 2022
Has anyone here been able to reproduce the AV with the changes in the PR and with the tests re-enabled? |
roji
commented
Sep 12, 2022
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. |
jkotas
commented
Sep 12, 2022
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.) |
danmoseley
commented
Sep 12, 2022
Just curious, what's the blocker on x86? |
ajcvickers
commented
Sep 13, 2022
@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. |
roji
commented
Sep 13, 2022
@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
commented
Sep 13, 2022
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. |
roji
commented
Sep 13, 2022
@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
commented
Sep 13, 2022
Sure @roji. Removing the "no merge" label. |
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.