Skip to content
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

Consider implementing Dispose pattern for handle typedefs #596

Closed
AArnott opened this issue Jun 24, 2022 · 11 comments
Closed

Consider implementing Dispose pattern for handle typedefs #596

AArnott opened this issue Jun 24, 2022 · 11 comments
Labels
enhancement New feature or request

Comments

@AArnott
Copy link
Member

AArnott commented Jun 24, 2022

When a typedef struct represents a releasable handle, we could implement IDisposable so that C# using syntax would allow deterministic release of these handle.

@AArnott AArnott added the enhancement New feature or request label Jun 24, 2022
@elachlan
Copy link
Contributor

I think this will be blocked by the solution in #610, which should establish how to reliably identify what nativedef structs are handles.

@AArnott
Copy link
Member Author

AArnott commented Jul 25, 2022

I don't think so... #610 is about picking which structs should get an IsNull property, but this issue only needs to know whether a given typedef struct has a releaser method, which we already know.

@elachlan
Copy link
Contributor

Yes, I guess we have to know HOW to release it, to be able to release it. Adding this won't be too bad.

@elachlan
Copy link
Contributor

Should this be able to be turned on/off by a setting? Or would there be no benefit in that?

@AArnott
Copy link
Member Author

AArnott commented Jul 26, 2022

I don't anticipate any downside to it, so I wouldn't add a switch unless a downside presents itself.

@elachlan
Copy link
Contributor

@AArnott if we add IDisposable to a struct, do we also need to add an bool ownsHandle similar to safehandles? That way we can handle when a method is tagged with DoNotReleaseAttribute?

@jnm2
Copy link
Contributor

jnm2 commented Aug 17, 2022

The handle struct would no longer be blittable and would also be subject to tearing.

@elachlan
Copy link
Contributor

So we can't use IDisposable then on any struct that is used by a method which has the DoNotReleaseAttribute.

@AArnott
Copy link
Member Author

AArnott commented Aug 18, 2022

No, it's not the IDisposable that would present the problems that @jnm2 calls out. It's the additional field where we would store the ownsHandle value.
If folks want .NET/C# to help with disposal of native resources, SafeHandle is the standard. The typedef structs are the native primitive that folks use when they specifically want to control lifetime themselves and/or cannot take the GC pressure of a SafeHandle. To make the struct disposable may be interesting, but it blurs the line. And if it led to analyzers producing warnings when they are not disposed of, that would violate a major point of falling back to them in the first place.

So maybe we could do the IDisposable struct thing if we new analyzers don't and would never call out disposing them. But I'm leaning against doing it at all given that's SafeHandle territory.

@elachlan
Copy link
Contributor

That is probably a good idea. So should we close this issue or limit it to structs without associated methods tagged with DoNotReleaseAttribute? I don't really have an opinion either way.

@AArnott
Copy link
Member Author

AArnott commented Aug 19, 2022

Given the risk that an analyzer exists or comes long that would try to coerce people to dispose of their typedef structs, I'd lean toward not implementing IDisposable, and continue to collect feedback.

@AArnott AArnott closed this as not planned Won't fix, can't repro, duplicate, stale Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants