-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add IDisposable interface to GCHandle #55596
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs
Outdated
Show resolved
Hide resolved
602b50c
to
461e817
Compare
461e817
to
904a3a9
Compare
/// <inheritdoc/> | ||
public void Dispose() | ||
{ | ||
IntPtr handle = _handle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ Can we not Interlocked.Exchange
here?
IntPtr handle = _handle; | |
IntPtr handle = Interlocked.Exchange(ref _handle, IntPtr.Zero); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
904a3a9
to
a29c100
Compare
The test failures here are real, e.g.
|
That's unexpected 🤔 EDIT: pushed a test as per Tanner's suggestion, let's see if that was actually it 👀 |
If it is the case, it would be useful to find the code that depends on the thread safety guarantees of Free. |
I suspect it's not a thread-safety thing; if it were, I'd expect it to be more non-deterministic. I pulled down the PR and deterministically get a failure here on every run:
I'll need to rebuild with a checked runtime, but presumably that internal error is related to the previously shown assert. |
|
@jkotas, does the runtime take a dependency on the method table / layout of GCHandle? If I just comment out |
The runtime has no dependencies like that. I think that the problem is going to be a code pattern like |
I think you're right... tracking it down... |
Heh, the problem is related to xunit theories. The xunit code adds every item yielded from a member data to this collection: and it then disposes of those: and we have tests that do things like this: Lines 122 to 129 in 3ec6907
Makes me a little nervous about this change. Obviously we can change our tests to not do that, but I'm not sure where else this pattern might creep up and lead to silent corruption. |
Cant the part where it disposes of them actually be patched to filter out GCHandles and dispose of them last (or not at all) as a patch to xunit itself? |
@AraHaan It'd be easier to remove the finally block from our test methods, relying on xunit to dispose of them instead of us performing manual disposal. Localized change, no xunit patch needed. But this doesn't address Steve's larger issue: "I'm not sure where else this pattern might creep up and lead to silent corruption." |
Maybe we can add Dispose without implementing the IDisposable interface? It won't make it possible to use GCHandle via using pattern, but it will at least fix the IsAllocated / Free dance that a lot of places have to do today. |
I think a better option is to create a new interface IDisposableHandle and use that for GCHandle and somehow hack the language to allow types implementing that interface to use "using"s with them. |
This is not worth a new interface in my opinion. If it really rose to a high level of importance, we could teach the language how to pattern match Dispose, just as it already can for ref structs. I'm fine with adding Dispose here without the interface implementation. I'm waffling on whether we should add the interface implementation, anyway; the test pattern here seems very unique to a combination of test harnesses and tests only we'd write in runtime, and this xunit approach would likely have issues with any type becoming IDisposable... this one just has harder to diagnose side effects. |
d457d9c
to
cf6976d
Compare
I think we might want to go back to the drawing board here and reconsider the API given the discovered semantics we've found. @jkotas 's suggestion is one option:
Or perhaps simply abandon this as too many patterns have been created over the years that make this troublesome to deal with. Similar to the |
@Sergio0694 Can we close this PR in lieu of the above discussion and concerns about compat? I think we need some additional thought on it. |
@AaronRobinsonMSFT Sure thing! 😄 ...I need to stop ever saying "this looks straightforward enough" when doing anything on the runtime 🤣 |
@Sergio0694 To be fair, lots of us thought it would be too. You are in good company. |
Closes #54792