Skip to content

Commit

Permalink
fix(ext/node): only set our end of child process pipe to nonblocking …
Browse files Browse the repository at this point in the history
…mode (#26495)

Fixes playwright on linux, as reported in
#16899 (comment).

The issue was that we were opening the socket in nonblocking mode, which
meant that subprocesses trying to use it would get a `EWOULDBLOCK` error
(unexpectedly). The fix here is to only set nonblocking mode on our end
(which we need to use asynchronously)
  • Loading branch information
nathanwhit authored Oct 24, 2024
1 parent fa49fd4 commit 7c57105
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 20 deletions.
21 changes: 9 additions & 12 deletions ext/io/bi_pipe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,10 @@ fn from_raw(
) -> Result<(BiPipeRead, BiPipeWrite), std::io::Error> {
use std::os::fd::FromRawFd;
// Safety: The fd is part of a pair of connected sockets
let unix_stream = tokio::net::UnixStream::from_std(unsafe {
std::os::unix::net::UnixStream::from_raw_fd(stream)
})?;
let unix_stream =
unsafe { std::os::unix::net::UnixStream::from_raw_fd(stream) };
unix_stream.set_nonblocking(true)?;
let unix_stream = tokio::net::UnixStream::from_std(unix_stream)?;
let (read, write) = unix_stream.into_split();
Ok((BiPipeRead { inner: read }, BiPipeWrite { inner: write }))
}
Expand Down Expand Up @@ -280,7 +281,7 @@ pub fn bi_pipe_pair_raw(
// https://github.com/nix-rust/nix/issues/861
let mut fds = [-1, -1];
#[cfg(not(target_os = "macos"))]
let flags = libc::SOCK_CLOEXEC | libc::SOCK_NONBLOCK;
let flags = libc::SOCK_CLOEXEC;

#[cfg(target_os = "macos")]
let flags = 0;
Expand All @@ -301,13 +302,13 @@ pub fn bi_pipe_pair_raw(
if cfg!(target_os = "macos") {
let fcntl = |fd: i32, flag: libc::c_int| -> Result<(), std::io::Error> {
// SAFETY: libc call, fd is valid
let flags = unsafe { libc::fcntl(fd, libc::F_GETFL) };
let flags = unsafe { libc::fcntl(fd, libc::F_GETFD) };

if flags == -1 {
return Err(fail(fds));
}
// SAFETY: libc call, fd is valid
let ret = unsafe { libc::fcntl(fd, libc::F_SETFL, flags | flag) };
let ret = unsafe { libc::fcntl(fd, libc::F_SETFD, flags | flag) };
if ret == -1 {
return Err(fail(fds));
}
Expand All @@ -323,13 +324,9 @@ pub fn bi_pipe_pair_raw(
std::io::Error::last_os_error()
}

// SOCK_NONBLOCK is not supported on macOS.
(fcntl)(fds[0], libc::O_NONBLOCK)?;
(fcntl)(fds[1], libc::O_NONBLOCK)?;

// SOCK_CLOEXEC is not supported on macOS.
(fcntl)(fds[0], libc::FD_CLOEXEC)?;
(fcntl)(fds[1], libc::FD_CLOEXEC)?;
fcntl(fds[0], libc::FD_CLOEXEC)?;
fcntl(fds[1], libc::FD_CLOEXEC)?;
}

let fd1 = fds[0];
Expand Down
2 changes: 1 addition & 1 deletion tests/specs/node/child_process_extra_pipes/main.out
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
data: hello world
[UNORDERED_START]
child closed
got: hello world
pipe closed
[UNORDERED_END]
20 changes: 16 additions & 4 deletions tests/specs/node/child_process_extra_pipes/main.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import child_process from "node:child_process";
import { Buffer } from "node:buffer";
import console from "node:console";

const child = child_process.spawn("./test-pipe/target/debug/test-pipe", [], {
Expand All @@ -8,19 +7,32 @@ const child = child_process.spawn("./test-pipe/target/debug/test-pipe", [], {

const extra = child.stdio[4];

const p = Promise.withResolvers();
if (!extra) {
throw new Error("no extra pipe");
}

const p = Promise.withResolvers<void>();

let got = "";

child.on("close", () => {
console.log("child closed");
p.resolve();
console.log("got:", got);
if (got === "hello world") {
p.resolve();
} else {
p.reject(new Error(`wanted "hello world", got "${got}"`));
}
});

extra.on("data", (d) => {
console.log("data:", d.toString().trim());
got += d.toString();
});

extra.on("close", () => {
console.log("pipe closed");
});

extra.write("start");

await p.promise;
25 changes: 22 additions & 3 deletions tests/specs/node/child_process_extra_pipes/test-pipe/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,31 @@
use std::fs::File;
use std::io::prelude::*;
use std::os::fd::FromRawFd;
use std::os::unix::net::UnixStream;

fn main() {
#[cfg(unix)]
{
let mut stream = unsafe { UnixStream::from_raw_fd(4) };
let mut pipe = unsafe { File::from_raw_fd(4) };

stream.write_all(b"hello world\n").unwrap();
let mut read = 0;
let mut buf = [0u8; 1024];
loop {
if read > 4 {
assert_eq!(&buf[..5], b"start");
break;
}
match pipe.read(&mut buf) {
Ok(n) => {
read += n;
}
Ok(0) => {
return;
}
Err(e) => {
eprintln!("GOT ERROR: {e:?}");
}
}
}
pipe.write_all(b"hello world").unwrap();
}
}

0 comments on commit 7c57105

Please sign in to comment.