Switch to windows-sys
#21
Conversation
src/lib.rs
Outdated
| /// On Windows a `ProcessHandle` is a `HANDLE`. | ||
| #[derive(Clone, Eq, PartialEq, Hash)] | ||
| pub struct ProcessHandle(Arc<ProcessHandleInner>); | ||
|
|
||
| impl Deref for ProcessHandle { | ||
| type Target = RawHandle; | ||
| type Target = HANDLE; |
There was a problem hiding this comment.
This changes the deref target, which means it's a breaking change. Considering there's no clear deref target anymore (windows-sys's HANDLE doesn't match RawHandle), there's three options:
- Making it deref to
HANDLE - Keep making it deref to
RawHandle(requires a bunch of unsafe code and idk if it's safe) - Remove the Deref impl altogether. The other platforms don't have it either
There was a problem hiding this comment.
Thanks for working on this. I'm inclined towards option 3 if we can verify that it doesn't break crates like remoteprocess. I don't have a Windows dev environment right now and will be depending on CI for testing, which I'll try to do soon.
(To connect a few dots, there's related discussion in #16 and #17.)
There was a problem hiding this comment.
There was a problem hiding this comment.
Made progress. remoteprocess's CI passes with these changes. The cast is a bit ugly and makes me wonder if there's a way to smooth it out on the read-process-memory end.
There was a problem hiding this comment.
I have to slightly be honest in that I partially did the switch to windows-sys because the previous use of raw pointers meant that read-process-memory was not thread safe, so while investigating that, I noticed that windows-sys solved these problems automatically anyway. So switching back to raw pointers would be suboptimal for those reasons. Maybe it would need manual Sync / Send impls then.
There was a problem hiding this comment.
I updated windows-sys again, added an assertion for Send / Sync and removed the Deref now.
There was a problem hiding this comment.
Thanks. I should have time in the next couple of weeks to look at this again.
There was a problem hiding this comment.
Okay, I made a fresh branch for remoteprocess here (the CI failures are safe to ignore). If anything looks odd or unnecessary, let me know.
I think we need to keep the Deref, at least for now -- remoteprocess needs access to the handle so that it can call other Windows APIs directly.
The casts still feel somewhat clunky, but I think that's a quirk of the ProcessHandle type and doesn't need to hold this up. I'll do more testing and will merge if everything looks good.
These are the official bindings that are maintained by Microsoft.
|
Thanks for your patience. I'm going to merge this, add the Deref back in (to support remoteprocess), and then work on migrating to OwnedHandle which seems like a good fit for us. |
These are the official bindings that are maintained by Microsoft.