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.

Sign up for GitHub

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

Jump to bottom

net: fix named pipe connect #5208

Merged
merged 5 commits into from Nov 20, 2022
Merged

net: fix named pipe connect #5208

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
90e5978
net: fix named pipe connect
Darksonn Nov 18, 2022
ca9cb48
fix named pipe
satakuma Nov 18, 2022
bdcf121
remove redundant loop
satakuma Nov 18, 2022
425bb69
add explicit pipe access test
satakuma Nov 19, 2022
7396f34
use oneshot channel instead of timing
satakuma Nov 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 7 additions & 9 deletions tokio/src/net/windows/named_pipe.rs
View file
Open in desktop
Expand Up @@ -192,17 +192,15 @@ impl NamedPipeServer {
/// # Ok(()) }
/// ```
pub async fn connect(&self) -> io::Result<()> {
loop {
match self.io.connect() {
Ok(()) => break,
Err(e) if e.kind() == io::ErrorKind::WouldBlock => {
self.io.registration().readiness(Interest::WRITABLE).await?;
}
Err(e) => return Err(e),
match self.io.connect() {
Err(e) if e.kind() == io::ErrorKind::WouldBlock => {
self.io
.registration()
.async_io(Interest::WRITABLE, || self.io.connect())
.await
}
x => x,
}

Ok(())
}

/// Disconnects the server end of a named pipe instance from a client
Expand Down
21 changes: 21 additions & 0 deletions tokio/tests/named_pipe.rs → tokio/tests/net_named_pipe.rs
View file
Open in desktop
Expand Up @@ -341,6 +341,27 @@ async fn test_named_pipe_mode_message() -> io::Result<()> {
Ok(())
}

// This tests `NamedPipeServer::connect` with various access settings.
#[tokio::test]
async fn test_named_pipe_access() -> io::Result<()> {
const PIPE_NAME: &str = r"\\.\pipe\test-named-pipe-access";

for (inb, outb) in [(true, true), (true, false), (false, true)] {
let server = tokio::spawn(async move {
let s = ServerOptions::new()
.access_inbound(inb)
.access_outbound(outb)
.create(PIPE_NAME)?;
s.connect().await
});
// Wait for the server to call connect.
time::sleep(Duration::from_millis(10)).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't have tests that depend on timing. They are brittle, and make the test suite take a long time. 10 ms sleeps add up to seconds surprisingly quickly if you have lots of tests.

You can do something like this:

let (tx, rx) = tokio::sync::oneshot::channel();
let server = tokio::spawn(async move {
    let s = ServerOptions::new()
        .access_inbound(inb)
        .access_outbound(outb)
        .create(PIPE_NAME)?;

    let mut connect_fut = tokio_test::task::spawn(s.connect());
    assert!(connect_fut.poll().is_pending());

    let _ = tx.send(());

    connect_fut.await.unwrap();
});

// Wait for the server to call connect.
rx.await.unwrap();
let _ = ClientOptions::new().read(outb).write(inb).open(PIPE_NAME)?;

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

let _ = ClientOptions::new().read(outb).write(inb).open(PIPE_NAME)?;
server.await??;
}
Ok(())
}

fn num_instances(pipe_name: impl AsRef<str>) -> io::Result<u32> {
use ntapi::ntioapi;
use winapi::shared::ntdef;
Expand Down