Skip to content

Commit

Permalink
dup and close file descriptors (#5341)
Browse files Browse the repository at this point in the history 
* track one shot fds

* dup fd

* skip for rearm on mac

* dup if fd

* cleanup

* force unregister on close

* deinitForceUnregister

* test

* add prompts

---------

Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
  • Loading branch information
@dylan-conway @Jarred-Sumner
dylan-conway and Jarred-Sumner authored Sep 15, 2023
1 parent f84fbd6 commit d26adde
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 35 deletions.
31 changes: 20 additions & 11 deletions src/bun.js/base.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1787,12 +1787,19 @@ pub const FilePoll = struct {

pub fn deinit(this: *FilePoll) void {
var vm = JSC.VirtualMachine.get();
this.deinitWithVM(vm);
var loop = vm.event_loop_handle.?;
this.deinitPossiblyDefer(vm, loop, vm.rareData().filePolls(vm), false);
}

pub fn deinitForceUnregister(this: *FilePoll) void {
var vm = JSC.VirtualMachine.get();
var loop = vm.event_loop_handle.?;
this.deinitPossiblyDefer(vm, loop, vm.rareData().filePolls(vm), true);
}

fn deinitPossiblyDefer(this: *FilePoll, vm: *JSC.VirtualMachine, loop: *uws.Loop, polls: *JSC.FilePoll.Store) void {
fn deinitPossiblyDefer(this: *FilePoll, vm: *JSC.VirtualMachine, loop: *uws.Loop, polls: *JSC.FilePoll.Store, force_unregister: bool) void {
if (this.isRegistered()) {
_ = this.unregister(loop);
_ = this.unregister(loop, force_unregister);
}

this.owner = Deactivated.owner;
Expand All @@ -1804,7 +1811,7 @@ pub const FilePoll = struct {

pub fn deinitWithVM(this: *FilePoll, vm: *JSC.VirtualMachine) void {
var loop = vm.event_loop_handle.?;
this.deinitPossiblyDefer(vm, loop, vm.rareData().filePolls(vm));
this.deinitPossiblyDefer(vm, loop, vm.rareData().filePolls(vm), false);
}

pub fn isRegistered(this: *const FilePoll) bool {
Expand Down Expand Up @@ -2168,10 +2175,12 @@ pub const FilePoll = struct {

var event = linux.epoll_event{ .events = flags, .data = .{ .u64 = @intFromPtr(Pollable.init(this).ptr()) } };

var op: u32 = if (this.isRegistered() or this.flags.contains(.needs_rearm)) linux.EPOLL.CTL_MOD else linux.EPOLL.CTL_ADD;

const ctl = linux.epoll_ctl(
watcher_fd,
if (this.isRegistered() or this.flags.contains(.needs_rearm)) linux.EPOLL.CTL_MOD else linux.EPOLL.CTL_ADD,
@as(std.os.fd_t, @intCast(fd)),
op,
@intCast(fd),
&event,
);
this.flags.insert(.was_ever_registered);
Expand Down Expand Up @@ -2285,11 +2294,11 @@ pub const FilePoll = struct {

const invalid_fd = bun.invalid_fd;

pub fn unregister(this: *FilePoll, loop: *uws.Loop) JSC.Maybe(void) {
return this.unregisterWithFd(loop, this.fd);
pub fn unregister(this: *FilePoll, loop: *uws.Loop, force_unregister: bool) JSC.Maybe(void) {
return this.unregisterWithFd(loop, this.fd, force_unregister);
}

pub fn unregisterWithFd(this: *FilePoll, loop: *uws.Loop, fd: bun.UFileDescriptor) JSC.Maybe(void) {
pub fn unregisterWithFd(this: *FilePoll, loop: *uws.Loop, fd: bun.UFileDescriptor, force_unregister: bool) JSC.Maybe(void) {
if (!(this.flags.contains(.poll_readable) or this.flags.contains(.poll_writable) or this.flags.contains(.poll_process) or this.flags.contains(.poll_machport))) {
// no-op
return JSC.Maybe(void).success;
Expand All @@ -2310,7 +2319,7 @@ pub const FilePoll = struct {
return JSC.Maybe(void).success;
};

if (this.flags.contains(.needs_rearm)) {
if (this.flags.contains(.needs_rearm) and !force_unregister) {
log("unregister: {s} ({d}) skipped due to needs_rearm", .{ @tagName(flag), fd });
this.flags.remove(.poll_process);
this.flags.remove(.poll_readable);
Expand All @@ -2325,7 +2334,7 @@ pub const FilePoll = struct {
const ctl = linux.epoll_ctl(
watcher_fd,
linux.EPOLL.CTL_DEL,
@as(std.os.fd_t, @intCast(fd)),
@intCast(fd),
null,
);

Expand Down
45 changes: 21 additions & 24 deletions src/bun.js/webcore/streams.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3788,7 +3788,6 @@ pub const FIFO = struct {
},
signal: JSC.WebCore.Signal = .{},
is_first_read: bool = true,
auto_close: bool = true,
has_adjusted_pipe_size_on_linux: bool = false,
drained: bool = true,

Expand All @@ -3807,16 +3806,20 @@ pub const FIFO = struct {
pub fn close(this: *FIFO) void {
if (this.poll_ref) |poll| {
this.poll_ref = null;
poll.deinit();
if (comptime Environment.isLinux) {
// force target fd to be removed from epoll
poll.deinitForceUnregister();
} else {
poll.deinit();
}
}

const fd = this.fd;
const signal_close = fd != bun.invalid_fd;
defer if (signal_close) this.signal.close(null);
if (signal_close) {
this.fd = bun.invalid_fd;
if (this.auto_close)
_ = bun.sys.close(fd);
_ = bun.sys.close(fd);
}

this.to_read = null;
Expand Down Expand Up @@ -4198,10 +4201,15 @@ pub const File = struct {
file: *Blob.FileStore,
) StreamStart {
var file_buf: [std.fs.MAX_PATH_BYTES]u8 = undefined;
var auto_close = file.pathlike == .path;

var fd = if (!auto_close)
file.pathlike.fd
var fd = if (file.pathlike != .path)
// We will always need to close the file descriptor.
switch (Syscall.dup(@intCast(file.pathlike.fd))) {
.result => |_fd| _fd,
.err => |err| {
return .{ .err = err.withPath(file.pathlike.path.slice()) };
},
}
else switch (Syscall.open(file.pathlike.path.sliceZ(&file_buf), std.os.O.RDONLY | std.os.O.NONBLOCK | std.os.O.CLOEXEC, 0)) {
.result => |_fd| _fd,
.err => |err| {
Expand All @@ -4218,7 +4226,7 @@ pub const File = struct {
}
}

if (!auto_close and !(file.is_atty orelse false)) {
if (file.pathlike != .path and !(file.is_atty orelse false)) {
if (comptime Environment.isWindows) {
bun.todo(@src(), {});
} else {
Expand All @@ -4229,7 +4237,6 @@ pub const File = struct {
// if we do not, clone the descriptor and set non-blocking
// it is important for us to clone it so we don't cause Weird Things to happen
if ((flags & std.os.O.NONBLOCK) == 0) {
auto_close = true;
fd = switch (Syscall.fcntl(fd, std.os.F.DUPFD, 0)) {
.result => |_fd| @as(@TypeOf(fd), @intCast(_fd)),
.err => |err| return .{ .err = err },
Expand All @@ -4249,24 +4256,18 @@ pub const File = struct {
const stat: bun.Stat = switch (Syscall.fstat(fd)) {
.result => |result| result,
.err => |err| {
if (auto_close) {
_ = Syscall.close(fd);
}
_ = Syscall.close(fd);
return .{ .err = err };
},
};

if (std.os.S.ISDIR(stat.mode)) {
if (auto_close) {
_ = Syscall.close(fd);
}
_ = Syscall.close(fd);
return .{ .err = Syscall.Error.fromCode(.ISDIR, .fstat) };
}

if (std.os.S.ISSOCK(stat.mode)) {
if (auto_close) {
_ = Syscall.close(fd);
}
_ = Syscall.close(fd);
return .{ .err = Syscall.Error.fromCode(.INVAL, .fstat) };
}

Expand All @@ -4292,9 +4293,7 @@ pub const File = struct {
file.max_size = this.remaining_bytes;

if (this.remaining_bytes == 0) {
if (auto_close) {
_ = Syscall.close(fd);
}
_ = Syscall.close(fd);

return .{ .empty = {} };
}
Expand All @@ -4303,7 +4302,6 @@ pub const File = struct {
}

this.fd = fd;
this.auto_close = auto_close;

return StreamStart{ .ready = {} };
}
Expand Down Expand Up @@ -4725,7 +4723,6 @@ pub const FileReader = struct {
.readable = .{
.FIFO = .{
.fd = readable_file.fd,
.auto_close = readable_file.auto_close,
.drained = this.buffered_data.len == 0,
},
},
Expand Down Expand Up @@ -4885,7 +4882,7 @@ pub fn NewReadyWatcher(
const fd = @as(c_int, @intCast(fd_));
std.debug.assert(@as(c_int, @intCast(this.poll_ref.?.fd)) == fd);
std.debug.assert(
this.poll_ref.?.unregister(JSC.VirtualMachine.get().event_loop_handle.?) == .result,
this.poll_ref.?.unregister(JSC.VirtualMachine.get().event_loop_handle.?, false) == .result,
);
}

Expand Down
Binary file modified test/bun.lockb
View file
Binary file not shown.
25 changes: 25 additions & 0 deletions test/js/third_party/prompts/prompts.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import prompt from "prompts";

const questions = [
{
type: "text",
name: "twitter",
message: `What's your twitter handle?`,
format: v => `@${v}`,
},
{
type: "number",
name: "age",
message: "How old are you?",
validate: value => (value < 18 ? `Sorry, you have to be 18` : true),
},
{
type: "password",
name: "secret",
message: "Tell me a secret",
},
];

const answers = await prompt(questions);

console.log(answers);
28 changes: 28 additions & 0 deletions test/js/third_party/prompts/prompts.test.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import path from "path";
import { bunExe, bunEnv } from "harness";

test("works with prompts", async () => {
var child = Bun.spawn({
cmd: [bunExe(), path.join(import.meta.dir, "prompts.js")],
env: bunEnv,
stdout: "pipe",
stdin: "pipe",
});

child.stdin.write("dylan\n");
Bun.sleepSync(100);
child.stdin.write("999\n");
Bun.sleepSync(100);
child.stdin.write("hi\n");

var out = "";
for await (const chunk of child.stdout) {
out += new TextDecoder().decode(chunk);
}

expect(await child.exited).toBe(0);

expect(out).toContain('twitter: "@dylan"');
expect(out).toContain("age: 999");
expect(out).toContain('secret: "hi"');
});
1 change: 1 addition & 0 deletions test/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"pg-connection-string": "2.6.1",
"postgres": "3.3.5",
"prisma": "5.1.1",
"prompts": "^2.4.2",
"socket.io": "4.7.1",
"socket.io-client": "4.7.1",
"supertest": "6.3.3",
Expand Down

0 comments on commit d26adde

Please sign in to comment.