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

add runtime pinner to pin unsafe pointer #77

Closed
wants to merge 1 commit into from

Conversation

changchaishi
Copy link

Fixes #74

Hi @jchv,

I'm unsure if this is how you wanted it, but I tried to add PinHandler for the handlers. I did not use function wrapping to avoid handling the type assertion.

PS: I am new to go.

@jchv
Copy link
Owner

jchv commented Jan 6, 2025

Thanks for taking a look at this. I am a bit too busy to properly review this, but I think this isn't quite ready to merge yet.

Firstly, the PinHanler function doesn't seem very useful. The calls to it may as well just call e.pinner.Pin directly instead.

Secondly, there's no call to e.pinner.Unpin. Leaking a Pinner will cause panics.

Finally, it might be better if there is some more general way to handle the runtime.Pinner for COM objects. I was thinking about the possibility of having each COM object have its own runtime.Pinner and call the Unpin when the refcount reaches zero. (Right now, I don't think we handle COM refcounting.) That said, there is a fair bit of nuance here so I haven't thought through all of the consequences. If you want to give it a go, though, feel free.

It isn't too easy to do automated testing with this library but we might need to find a way. Some of these behaviors (e.g. not panicking on GC finalization) could and probably should be tested, but there are no suitable tests to catch it yet.

@jchv
Copy link
Owner

jchv commented Jan 6, 2025

(sidenote: the lint error indicates that I need to update CI.)

@changchaishi
Copy link
Author

I am not ready for this kind of change. I close the PR first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use runtime.Pin to remove unsafe code
2 participants