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

build: update windows-sys to 0.59.0 #1857

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ErichDonGubler
Copy link

Carries on the effort from #1820, with some fixes that should make tests compile now.

Comment on lines 76 to 77
unsafe impl Send for NamedPipe {}
unsafe impl Sync for NamedPipe {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which field in Inner is making this necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may make more sense to implement these traits for Inner instead, or to wrap the offending field in a newtype and implement them on that newtype.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which field in Inner is making this necessary?

We need to implement Send and Sync manually because HANDLE is no longer considered thread-safe. I think your instinct is right, and we need to capture the overlapped I/O contract with types ourselves now.

Will take some time to dig into that contract, and report what I learn here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems much easier to reason about the specific APIs' usage of Handles, rather than a category of resource backing a Handle, for the purpose of justifying unsafe impl {Send,Sync}. I'll implement Send and Sync directly on iocp::CompletionPort and named_pipe::Inner first.

If there's desire to attempt categorizing types of resources behind Handles as thread-safe, maybe a follow-up PR would be best?

@Darksonn
Copy link
Contributor

There's a build failure.

     Checking mio v1.0.3 (D:\a\mio\mio)
  error[E0425]: cannot find function `null_mut` in this scope
    --> src\sys\windows\afd.rs:69:13
     |
  69 |             null_mut(),
     |             ^^^^^^^^ not found in this scope
     |
  help: consider importing this function
     |
  1  + use std::ptr::null_mut;
     |

@ErichDonGubler
Copy link
Author

ErichDonGubler commented Jan 22, 2025

@Darksonn:

There's a build failure.

     Checking mio v1.0.3 (D:\a\mio\mio)
  error[E0425]: cannot find function `null_mut` in this scope
    --> src\sys\windows\afd.rs:69:13
     |
  69 |             null_mut(),
     |             ^^^^^^^^ not found in this scope
     |
  help: consider importing this function
     |
  1  + use std::ptr::null_mut;
     |

Ah, that was an interesting feature combination that I had to use to reproduce locally. 😅 Glad CI caught it!

@Snowiiii
Copy link
Contributor

Any progress here ?

@ErichDonGubler
Copy link
Author

@Snowiiii: None, until you'd asked, and I'd refreshed my memory on this PR. 😀

I've updated the contents of the PR and its conversations with my latest thinking. I think this is ready for a re-review from @Darksonn.

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.

4 participants