-
Notifications
You must be signed in to change notification settings - Fork 86
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
Reconsider taking HandeRef as input parameters when the parameter must be a handle of some sort. #202
Comments
Use within the runtime itself is not an argument that applies to this repo as the runtime repo is not our target audience. We need a good reason to generate what was deprecated so long ago. Can you explain a scenario where |
@AArnott Again, this is for scenarios where you do not have control of the code that created the handle, such as |
oh and speaking of which it's also for when you need to use the underlying os apis to do extra work that the Windows Forms library does not do (like calling undocumented apis to make every control and the window itself dark instead of light like how Windows Explorer does). I am however tempted to change that eventually by making an fork then apply the logic from here to make everything even the scrollbars dark which would be epic to do then file an issue for it to be approved first then if approved it being pull requested in to winforms (and possible wpf as well) that way it can abstract away the need to make the stuff dark when dark mode on windows is enabled (everything then every single context menu strip, etc would be made dark then). Plus I even know how to darkify the system menu strip too on the forms (however needs handles after first converting the original ones to a ContextMenuStrip to the winforms library with an dark mode renderer that renders as dark only when dark is set) and then set that strip as the system menu strip. So yes, it's useful for things like However I do not know if the Windows Forms or WPF teams would welcome the use of undocumented windows apis being used inside of those libraries despite it now being easy to be able to do. |
I'm sure they wouldn't, on legal grounds. Microsoft products outside Windows cannot call into undocumented Windows APIs.
Why do you say that? Who is going to finalize a
Well,
If WinForms has public APIs that produce HandleRef for you to use and you want to use that in CsWin32-generated APIs, then that's a more solid case. If those APIs exist, can you produce examples? |
@AArnott I must have not been communicating clearly. I'm not sure how to reply to most of your responses. Here's the situation that HandleRef is useful for, and there's simply no way that I can ever use SafeHandle to help prevent void DoSomething(Form form)
{
// Danger! The GC might finalize 'form' during this call, freeing the handle.
User32.EnumChildWindows(form.Handle, ..., ...);
} Seeing HandleRef and not being able to pass just void DoSomething(Form form)
{
User32.EnumChildWindows(new HandleRef(form, form.Handle), ..., ...);
} This is also safe, but I hardly ever remember to add the GC.KeepAlive call. I think this is what @AraHaan is asking when they say either make it a HandleRef overload so you can't forget about this problem, or add in the XML docs that you should add GC.KeepAlive if the handle is owned by a managed object. void DoSomething(Form form)
{
User32.EnumChildWindows(form.Handle, ..., ...);
GC.KeepAlive(form);
} SafeHandle makes no sense here because the void DoSomething(Form form)
{
// Nothing is stopping the GC from concurrently finalizing 'form' and freeing the handle during this call
User32.EnumChildWindows(new SomeKindOfSafeHandle(form.Handle), ..., ...);
} |
You could say "the problem is clearly that Windows Forms and similar libraries should be using SafeHandle," but that doesn't solve the question of what to do with today's .NET Core 3.0–.NET 6's Windows Forms, or what to do with .NET Framework's Windows Forms which will probably never deprecate its IntPtr handle property even if .NET 10 does. The problem is so systemic that |
That may be my fault. I've never use And thank you for your last couple comments. I feel like I understand the scenario much better now.
Either of these options sound reasonable. Given this seems to be a problem specifically with WinForms, if we offer HandleRef overloads I think it will only be under a non-default option since it appeals only to a subset of the crowd. |
Okay, cool! I'm glad that helped. I think making HandleRef opt-in is very reasonable since it's only there to cope with non-SafeHandle finalizers, and Microsoft docs were finally updated last year to start recommending SafeHandle instead of freeing handles directly in handwritten finalizers. At the bottom of the docs seems fine. This is almost better suited to be reported as a warning by the source generator than to be in docs though. |
That's an interesting idea. Basically anytime an |
Yes. Now that I'm thinking about it, you'd probably need to expose an analyzer because the source generator probably can't itself be the one to register for IInvocationOperation/IArgumentOperations etc to check and provide warnings for all call sites. (Or even if it could, you wouldn't want to delay the source generator by eagerly demanding parsing and semantics to be fully generated for the whole project. Though maybe they are already available before any source is generated?) |
Oh, also, separate analyzer would be analyzing a semantic model of the compilation that already has the generated source, which probably makes everything easier than dealing with missing types and members. |
* Crank up dependabot * Bump Microsoft.NET.Test.Sdk from 17.5.0 to 17.6.0 Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 17.5.0 to 17.6.0. - [Release notes](https://github.com/microsoft/vstest/releases) - [Changelog](https://github.com/microsoft/vstest/blob/main/docs/releases.md) - [Commits](microsoft/vstest@v17.5.0...v17.6.0) --- updated-dependencies: - dependency-name: Microsoft.NET.Test.Sdk dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Andrew Arnott <andrewarnott@live.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
I don't see such a finalizer. There is Form.Dispose(bool), which indirectly calls Control.Dispose(bool), but that calls DestroyHandle only if the disposing parameter is Open forms cannot be finalized anyway, as they are rooted from the System.Windows.Forms.Application.OpenForms collection. |
Currently in CsWin32 no overloads taking in
HandleRef
s are generated.The use cases:
HandleRef
is not only used by user applications (for referencingHWND
handles to (most commonly) gui controls to pass them into p/invoke methods to the operating system apis).HandeRef
for p/invokes to handles they do not own themselves to keep the GC from destroying them in mid call to the p/invoke.This is where my code uses it: https://github.com/Elskom/Els_kom_new/search?q=HandleRef&type=code (it seems to not show every usage of it for some reason in my NativeMethods.cs file.)
Got 616 results for it as well too https://github.com/search?q=org%3Adotnet+HandleRef&type=code, a lot of commits, and issues regarding it as well.
More results could be in the code bases within all of the repositories in the dotnet organization on github, one way to find out for sure how many results come up is to clone them all, and then within Visual Studio search for
HandleRef
in every last repository cloned from there to get the absolute number of usages in the code within that organization.I also encourage an in general look to see how many other people might also use
HandleRef
as well too.As seen above,
HandleRef
while documented as deprecated in .NET 2.0, is still widely used everywhere even within the runtime itself with valid use cases that make it impossible to use say SafeHandle, expecially when you own the object that made the handle but not the handle itself and as such usingHandleRef
-orGC.KeepAlive(objectmadeinthiscode);
are the only options to force the GC to not finalize the object (with said handle), before the native operating system calls finish with it and possibly cause undefined behavior on user's applications (and possibly the runtime itself as well).This is why one or the other should be done, either tell the user to
tell the GC to keep that object alive.
or use aHandeRef
that would need to be generated by CsWin32.Another option for this, is to have it not generate
HandleRef
overloads by default but have the user opt-in for them if they specifically ask for said overload to be generated within the configuration for specific members only (opt-in per p/invoke function name) within the text file that controls the source generator.There was more discussion about this in #125.
The text was updated successfully, but these errors were encountered: