[windows] IPC #8497
[windows] IPC #8497
Conversation
cirospaciari
force-pushed
the
ciro/win-ipc
branch
from
9053dab to
6f75cb1
Compare
|
❌ @cirospaciari 4 files with test failures on bun-darwin-aarch64:
|
|
❌ @cirospaciari 4 files with test failures on linux-x64: |
|
❌ @cirospaciari 4 files with test failures on linux-x64-baseline: |
|
❌ @cirospaciari 6 files with test failures on bun-darwin-x64:
|
src/bun.js/ipc.zig
Outdated
| log("processSend {d}", .{bytes.len}); | ||
| if (bytes.len == 0) return; | ||
|
|
||
| const req = bun.new(uv.uv_write_t, std.mem.zeroes(uv.uv_write_t)); |
There was a problem hiding this comment.
is this necessary? Can we use uv_try_write and be notified when the pipe is writable instead? That would simplify this code and reduce memory allocations.
There was a problem hiding this comment.
Did almost that, libuv dont have a way to know when is writable but I did something similiar that we already did on subprocess.
src/bun.js/ipc.zig
Outdated
| } | ||
| } | ||
|
|
||
| fn uvWriteCallback(req: *uv.uv_write_t, status: uv.ReturnCode) callconv(.C) void { |
There was a problem hiding this comment.
If we do use this approach - uv.uv_write, we should have a wrapper so the arguments are this: *NamedIPCPipeData instead of having to deal with the internals of libuv everywhere
| fn uvWriteCallback(req: *uv.uv_write_t, status: uv.ReturnCode) callconv(.C) void { | |
| fn onWriteComplete(this: *NamedIPCPipeData, result: union(enum) { written: u32, err: bun.sys.Error}) callconv(.C) void { |
There was a problem hiding this comment.
We dont have a reliable internal way to see how many bytes was written only using the libuv struct (to avoid allocations) on windows, so I did fn onWriteComplete(this: *NamedPipeIPCData, status: uv.ReturnCode) void but is already way better
❌🪟 @cirospaciari, there are 5 test regressions on Windows x86_64
|
cirospaciari
force-pushed
the
ciro/win-ipc
branch
from
d7fc6e0 to
3a5d83a
Compare
@Jarred-Sumner Added wrappers, now I'm using try_write, and using uv_write as mechanism to know when we are writable, and fixed the memory issue. |
|
@cirospaciari there are merge conflicts |
|
|
||
| const rc = uv_write(req, stream, @ptrCast(&req.write_buffer), 1, &Wrapper.uvWriteCb); | ||
| if (rc.errno()) |errno| { | ||
| return .{ .err = .{ .errno = errno, .syscall = .write, .from_libuv = true } }; |
There was a problem hiding this comment.
the from_uv's here will cause incorrect error messages
| writer.flush(); | ||
| } else { | ||
| // echo | ||
| const writer = Bun.stdout.writer(); |
There was a problem hiding this comment.
| const writer = Bun.stdout.writer(); | |
| for await (let chunk of Bun.stdin) { | |
| console.write(chunk); | |
| } |
| process.exit(0); | ||
| } | ||
| case "cat": { | ||
| if (fs.existsSync(argument)) { |
There was a problem hiding this comment.
| if (fs.existsSync(argument)) { | |
| await Bun.write(Bun.stdout, Bun.file(argument)) |
cirospaciari
force-pushed
the
ciro/win-ipc
branch
from
b9318f8 to
f5f71e5
Compare
| if (map.map.fetchSwapRemove("BUN_INTERNAL_IPC_FD")) |kv| { | ||
| if (std.fmt.parseInt(i32, kv.value.value, 10) catch null) |fd| { | ||
| this.initIPCInstance(bun.toFD(fd)); | ||
| if (map.map.fetchSwapRemove("BUN_INTERNAL_IPC_PIPE")) |kv| { |
There was a problem hiding this comment.
is there a reason we cant use the same environment variable as it is on posix.
one lookup instead of two?
|
Closed in favor of #8456 |
What does this PR do?
How did you verify your code works?
Existing Tests