feat: support named pipe listen, connect and open#31624
feat: support named pipe listen, connect and open#31624bartlomieju merged 1 commit intodenoland:mainfrom
Conversation
WalkthroughAdds Windows named-pipe support to the net extension and Node pipe polyfill. Introduces Windows-gated modules Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
ext/net/win_pipe.rs (1)
9-9: Unused import.
AsyncResultis imported but never used in this file. The methods returnio::Result<_>directly.-use deno_core::AsyncResult;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ext/net/lib.rs(3 hunks)ext/net/ops_win_pipe.rs(1 hunks)ext/net/win_pipe.rs(1 hunks)ext/node/polyfills/internal_binding/pipe_wrap.ts(8 hunks)tests/node_compat/config.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
ext/node/polyfills/internal_binding/pipe_wrap.ts
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
ext/net/ops_win_pipe.rsext/net/win_pipe.rsext/net/lib.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
ext/net/ops_win_pipe.rsext/net/win_pipe.rsext/net/lib.rs
ext/**/lib.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Extensions should provide ops (operations) exposed to JavaScript in Rust code within
ext/<extension_name>/directories
Files:
ext/net/lib.rs
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to ext/**/lib.rs : Extensions should provide ops (operations) exposed to JavaScript in Rust code within `ext/<extension_name>/` directories
Applied to files:
ext/net/lib.rs
🧬 Code graph analysis (3)
ext/node/polyfills/internal_binding/pipe_wrap.ts (3)
ext/node/polyfills/internal_binding/stream_wrap.ts (2)
StreamBase(75-75)kStreamBaseField(121-121)ext/net/lib.rs (3)
op_pipe_connect(240-247)op_pipe_open(229-236)op_pipe_windows_wait(24-26)ext/net/ops_win_pipe.rs (3)
op_pipe_connect(106-152)op_pipe_open(64-91)op_pipe_windows_wait(94-102)
ext/net/ops_win_pipe.rs (2)
ext/net/lib.rs (2)
op_pipe_open(229-236)state(44-45)ext/net/win_pipe.rs (2)
new_server(29-38)new_client(40-49)
ext/net/lib.rs (1)
ext/net/ops_win_pipe.rs (3)
op_pipe_windows_wait(94-102)op_pipe_open(64-91)op_pipe_connect(106-152)
🪛 GitHub Check: lint debug windows-x86_64
ext/node/polyfills/internal_binding/pipe_wrap.ts
[failure] 245-245:
�[0m�[1mDon't use the global intrinsic
[failure] 228-228:
�[0m�[1mDon't use the global intrinsic
[failure] 228-228:
�[0m�[1mDon't use the global intrinsic
[failure] 226-226:
�[0m�[1mDon't use the global intrinsic
[failure] 208-208:
�[0m�[1mDon't use the global intrinsic
[failure] 471-471:
�[0m�[1me is never used
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
🔇 Additional comments (6)
tests/node_compat/config.toml (1)
812-812: LGTM!Test entry correctly placed in alphabetical order with default configuration.
ext/net/win_pipe.rs (1)
79-91: LGTM!The
Resourceimplementation correctly usesimpl_readable_byob!andimpl_writable!macros, and the close method cancels pending operations. The underlying pipe will be properly closed when theRc<NamedPipe>is dropped.ext/net/ops_win_pipe.rs (4)
93-102: LGTM!The async op correctly retrieves the resource, drops the state borrow before awaiting, and propagates errors appropriately.
111-134: LGTM!Good defensive path detection covering both backslash and forward slash variants. The error handling for non-pipe paths correctly returns ENOTSOCK for existing files and ENOENT for missing paths.
62-91: LGTM!Server pipe creation with proper permission checks, configurable options, and resource table registration.
85-87: No type mismatch formax_instances.Both
ListenArgs.max_instancesandServerOptions::max_instances()useusize, so there's no type conversion issue. However,tokio'smax_instances()accepts values 1-254 and panics if exceeded. Consider validating the input range before passing it to the API.Likely an incorrect or invalid review comment.
e14d56b to
0fdf00e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ext/node/polyfills/internal_binding/pipe_wrap.ts (2)
62-94:ref()andunref()are no-ops - flag is set but never used.The
#refedflag is toggled but never read or used to actually ref/unref the underlying resource. This meansunref()won't prevent the process from exiting while the pipe is active. Consider usingcore.refOpPromise()/core.unrefOpPromise()or similar mechanisms to implement actual ref behavior.
451-483: Backoff delay never reset on success and never escalates on error.Two issues persist from prior review:
- After a successful accept (line 466),
#acceptBackoffDelayis never reset, so a previous error's delay persists.- In the catch block (line 495), the delay never escalates - always uses
INITIAL_ACCEPT_BACKOFF_DELAY.ext/net/lib.rs (1)
211-227: Stub op signatures don't match Windows implementations.The Windows
op_pipe_openandop_pipe_connectaccept(state, args, api_name)parameters, but these stubs accept none. While this works at runtime (stubs just return errors), the signature mismatch could cause issues with tooling or future deno_core changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ext/net/lib.rs(3 hunks)ext/net/ops_win_pipe.rs(1 hunks)ext/net/win_pipe.rs(1 hunks)ext/node/polyfills/internal_binding/pipe_wrap.ts(8 hunks)tests/node_compat/config.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/node_compat/config.toml
- ext/net/win_pipe.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
ext/node/polyfills/internal_binding/pipe_wrap.ts
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
ext/net/ops_win_pipe.rsext/net/lib.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
ext/net/ops_win_pipe.rsext/net/lib.rs
ext/**/lib.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Extensions should provide ops (operations) exposed to JavaScript in Rust code within
ext/<extension_name>/directories
Files:
ext/net/lib.rs
🧠 Learnings (1)
📚 Learning: 2025-11-20T09:12:20.198Z
Learnt from: marvinhagemeister
Repo: denoland/deno PR: 31358
File: ext/node/polyfills/process.ts:759-766
Timestamp: 2025-11-20T09:12:20.198Z
Learning: In ext/node/polyfills/process.ts, the process.sourceMapsEnabled property should be defined with enumerable: true to match Node.js behavior (as seen in Node.js source: lib/internal/bootstrap/node.js).
Applied to files:
ext/node/polyfills/internal_binding/pipe_wrap.ts
🧬 Code graph analysis (3)
ext/node/polyfills/internal_binding/pipe_wrap.ts (3)
ext/node/polyfills/internal_binding/stream_wrap.ts (2)
StreamBase(75-75)kStreamBaseField(121-121)ext/net/lib.rs (3)
op_pipe_connect(222-227)op_pipe_open(213-218)op_pipe_windows_wait(230-235)ext/net/ops_win_pipe.rs (3)
op_pipe_connect(106-150)op_pipe_open(64-91)op_pipe_windows_wait(94-102)
ext/net/ops_win_pipe.rs (2)
ext/net/lib.rs (3)
op_pipe_open(213-218)op_pipe_windows_wait(230-235)op_pipe_connect(222-227)ext/net/win_pipe.rs (2)
new_server(29-38)new_client(40-49)
ext/net/lib.rs (1)
ext/net/ops_win_pipe.rs (2)
op_pipe_open(64-91)op_pipe_connect(106-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (9)
ext/net/ops_win_pipe.rs (4)
19-21: Dead code:is_truefunction appears unused.This helper function is used as a default value provider for serde, but
#[serde(default = "is_true")]on bool fields means they default totrueonly when the field is missing. This is correct usage, so the function is actually used.
23-38: LGTM!Clean enum definition with proper serde rename attributes and a straightforward
Fromimplementation for conversion to tokio'sPipeMode.
62-91: LGTM!Permission check before opening the pipe, proper configuration of server options, and clean resource table integration.
93-102: LGTM!Correct pattern: acquire the resource, drop the state borrow, then await the async operation.
ext/node/polyfills/internal_binding/pipe_wrap.ts (3)
197-268: LGTM!Windows connect flow correctly uses primordials for
queueMicrotaskandStringPrototypeIncludes. Error mapping to UV codes is thorough.
314-338: LGTM!Windows listen flow correctly creates the server pipe and starts the accept loop.
564-571: LGTM!Proper cleanup of
#serverPipeRidon close with appropriate error swallowing for already-closed resources.ext/net/lib.rs (2)
8-16: LGTM!Windows-specific modules correctly gated behind
#[cfg(windows)].
105-108: LGTM!Pipe ops correctly registered in the extension.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/node/polyfills/internal_binding/pipe_wrap.ts (1)
389-399: ref()/unref() are no-ops on Windows.On Windows,
this.#listeneris never set (the listen path uses#serverPipeRidinstead), so these methods do nothing. This meansserver.unref()won't allow the process to exit while the Windows server is listening, breaking expected behavior.Consider tracking the
op_pipe_windows_waitpromise in#acceptWindowsand applying ref/unref to it, similar to how individual connections handle this.
♻️ Duplicate comments (1)
ext/node/polyfills/internal_binding/pipe_wrap.ts (1)
517-530: Backoff delay is not escalated on errors.The error path uses a fixed delay (
this.#acceptBackoffDelay || INITIAL_ACCEPT_BACKOFF_DELAY) but never increments the backoff on repeated failures. Compare to the Unix path which calls#acceptBackoff()(lines 449-466) to exponentially increase the delay up to the max.Apply this diff to escalate backoff on errors:
} catch { if (this.#closed) { return; } try { this.onconnection!(MapPrototypeGet(codeMap, "UNKNOWN")!, undefined); } catch { // swallow callback errors. } + // Escalate backoff delay + if (!this.#acceptBackoffDelay) { + this.#acceptBackoffDelay = INITIAL_ACCEPT_BACKOFF_DELAY; + } else { + this.#acceptBackoffDelay = Math.min( + this.#acceptBackoffDelay * 2, + MAX_ACCEPT_BACKOFF_DELAY, + ); + } + await delay(this.#acceptBackoffDelay || INITIAL_ACCEPT_BACKOFF_DELAY); return this.#acceptWindows(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ext/node/polyfills/internal_binding/pipe_wrap.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
ext/node/polyfills/internal_binding/pipe_wrap.ts
🧠 Learnings (1)
📚 Learning: 2025-11-20T09:12:20.198Z
Learnt from: marvinhagemeister
Repo: denoland/deno PR: 31358
File: ext/node/polyfills/process.ts:759-766
Timestamp: 2025-11-20T09:12:20.198Z
Learning: In ext/node/polyfills/process.ts, the process.sourceMapsEnabled property should be defined with enumerable: true to match Node.js behavior (as seen in Node.js source: lib/internal/bootstrap/node.js).
Applied to files:
ext/node/polyfills/internal_binding/pipe_wrap.ts
🧬 Code graph analysis (1)
ext/node/polyfills/internal_binding/pipe_wrap.ts (4)
ext/node/polyfills/internal_binding/stream_wrap.ts (2)
StreamBase(75-75)kStreamBaseField(121-121)ext/net/lib.rs (3)
op_pipe_connect(222-227)op_pipe_open(213-218)op_pipe_windows_wait(230-235)ext/net/ops_win_pipe.rs (3)
op_pipe_connect(106-150)op_pipe_open(64-91)op_pipe_windows_wait(94-102)ext/node/polyfills/internal_binding/_listen.ts (1)
MAX_ACCEPT_BACKOFF_DELAY(20-20)
🔇 Additional comments (4)
ext/node/polyfills/internal_binding/pipe_wrap.ts (4)
73-127: LGTM! WindowsNamedPipeStream implementation is solid.The class correctly wraps the Windows named pipe RID, implements the StreamBase interface, and properly manages ref/unref behavior by tracking pending read promises.
220-291: LGTM! Windows connect implementation is thorough.The Windows-specific connection logic properly uses the pipe ops, creates the stream wrapper, and has comprehensive error mapping that handles various error formats (code, message patterns, rawOsError).
337-361: LGTM! Windows listen implementation is correct.The Windows path properly opens the server pipe, stores the RID for later cleanup, and initiates the accept loop. Error handling appropriately rethrows NotCapable errors.
597-604: LGTM! Windows server pipe cleanup is correct.Properly closes the server pipe RID when the server is a PIPESERVERWRAP, handles already-closed cases, and clears the RID field.
54d2263 to
c2f159f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/node/polyfills/internal_binding/pipe_wrap.ts (1)
448-463: Method does too much and calls the wrong accept method.
#setNextBackoffDelayescalates the delay, awaits it, then callsthis.#accept()(the Unix accept). When called from#acceptWindowson line 501, this would invoke the wrong accept method. The method should either:
- Only set the delay value without calling accept (rename to match behavior)
- Be split into separate methods for setting delay and continuing accept
Additionally, the method name implies it only "sets" the delay, but it also performs the delay and continues the accept loop.
♻️ Duplicate comments (2)
ext/node/polyfills/internal_binding/pipe_wrap.ts (2)
500-501: Backoff logic is inverted - escalating on success.Line 501 calls
#setNextBackoffDelay()after a successful accept, but that method escalates the delay (increases it). The comment says "Reset" but the code does the opposite. On success, you should reset the backoff like the Unix path does on line 573:- // Reset the backoff delay upon successful accept. - this.#setNextBackoffDelay(); + // Reset the backoff delay upon successful accept. + this.#acceptBackoffDelay = undefined;
519-532: Missing backoff escalation on repeated errors.The error path delays with
this.#acceptBackoffDelay || INITIAL_ACCEPT_BACKOFF_DELAYbut never increments the backoff, so repeated failures will keep using the same delay. Add escalation logic:} catch { if (this.#closed) { return; } try { this.onconnection!(MapPrototypeGet(codeMap, "UNKNOWN")!, undefined); } catch { // swallow callback errors. } + + // Escalate backoff on repeated failures + if (!this.#acceptBackoffDelay) { + this.#acceptBackoffDelay = INITIAL_ACCEPT_BACKOFF_DELAY; + } else { + this.#acceptBackoffDelay = Math.min( + this.#acceptBackoffDelay * 2, + MAX_ACCEPT_BACKOFF_DELAY, + ); + } - await delay(this.#acceptBackoffDelay || INITIAL_ACCEPT_BACKOFF_DELAY); + await delay(this.#acceptBackoffDelay); return this.#acceptWindows(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ext/node/polyfills/internal_binding/pipe_wrap.ts(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
ext/node/polyfills/internal_binding/pipe_wrap.ts
🧠 Learnings (1)
📚 Learning: 2025-11-20T09:12:20.198Z
Learnt from: marvinhagemeister
Repo: denoland/deno PR: 31358
File: ext/node/polyfills/process.ts:759-766
Timestamp: 2025-11-20T09:12:20.198Z
Learning: In ext/node/polyfills/process.ts, the process.sourceMapsEnabled property should be defined with enumerable: true to match Node.js behavior (as seen in Node.js source: lib/internal/bootstrap/node.js).
Applied to files:
ext/node/polyfills/internal_binding/pipe_wrap.ts
🧬 Code graph analysis (1)
ext/node/polyfills/internal_binding/pipe_wrap.ts (3)
ext/node/polyfills/internal_binding/stream_wrap.ts (2)
StreamBase(75-75)kStreamBaseField(121-121)ext/net/ops_win_pipe.rs (3)
op_pipe_connect(106-150)op_pipe_open(64-91)op_pipe_windows_wait(94-102)ext/net/lib.rs (3)
op_pipe_connect(222-227)op_pipe_open(213-218)op_pipe_windows_wait(230-235)
🔇 Additional comments (5)
ext/node/polyfills/internal_binding/pipe_wrap.ts (5)
27-71: Imports and primordials look good.All the new imports for Windows pipe operations and primordials are properly used throughout the implementation.
73-127: WindowsNamedPipeStream implementation is solid.The class correctly implements the StreamBase interface with proper RID management, ref/unref behavior using core APIs, and primordials throughout. The pending read promise tracking ensures unref works correctly.
219-291: Windows connect implementation handles errors well.The Windows-specific connection path properly uses op_pipe_connect, wraps the RID in WindowsNamedPipeStream, and includes comprehensive error mapping that covers various Windows error scenarios including raw OS error codes.
332-387: Windows listen implementation looks correct.Properly opens a Windows named pipe server with op_pipe_open, stores the RID, and starts the Windows-specific accept loop. Error handling correctly distinguishes capability errors from other failures.
587-616: Windows resource cleanup is correct.The
_onClosemethod properly cleans up the Windows server pipe RID usingcore.close, handles already-closed resources gracefully, and clears the reference.
ext/net/ops_win_pipe.rs
Outdated
| #[smi] rid: ResourceId, | ||
| ) -> Result<(), NetError> { | ||
| let pipe = state.borrow().resource_table.get::<NamedPipe>(rid)?; | ||
| drop(state); |
| "parallel/test-net-persistent-keepalive.js" = {} | ||
| "parallel/test-net-persistent-nodelay.js" = {} | ||
| "parallel/test-net-persistent-ref-unref.js" = {} | ||
| "parallel/test-net-pipe-connect-errors.js" = {} |
There was a problem hiding this comment.
Nice, have you tried enabling any other tests? You can quickly check if other tests are passing with deno run -A ./tools/node_compat_tests.js --filter parallel/test-net-pipe
There was a problem hiding this comment.
I think that only there are parallel/test-net-pipe-connect-errors.js and parallel/test-net-pipe-with-long-path.js with this prefix. But parallel/test-net-pipe-with-long-path.js was already enabled.
|
That's great. It's still under review, but would it be better to include @CyanChanges as a co-author? |
be4eadc to
ebacf76
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ext/net/ops_win_pipe.rs (1)
77-108: Permission check ordering is correct, but consider using canonical path for existence check.The permission check now properly precedes the
exists()call, addressing the previous security concern. However, lines 98-99 check the originalpathstring rather thanchecked_path(the canonicalized path from the permission check). For consistency, consider checkingchecked_path.exists()since that's the authorized, canonical path you'll actually use at line 113.Apply this diff:
if !is_named_pipe { // For non-pipe paths, check if the path exists as a file // If it does, return ENOTSOCK (not a socket) // If it doesn't exist, return ENOENT - let path = Path::new(&path); - if path.exists() { + if checked_path.exists() { return Err(NetError::Io(std::io::Error::other( "ENOTSOCK: not a socket", ))); } else { return Err(NetError::Io(std::io::Error::other( "ENOENT: no such file or directory", ))); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ext/net/ops_win_pipe.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
ext/net/ops_win_pipe.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
ext/net/ops_win_pipe.rs
🧬 Code graph analysis (1)
ext/net/ops_win_pipe.rs (2)
ext/net/lib.rs (3)
op_pipe_open(213-218)op_pipe_windows_wait(230-235)op_pipe_connect(222-227)ext/net/win_pipe.rs (4)
new_server(29-38)read(69-76)write(60-67)new_client(40-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test release linux-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: build libs
- GitHub Check: lint debug windows-x86_64
🔇 Additional comments (3)
ext/net/ops_win_pipe.rs (3)
18-56: LGTM!Permission check is performed before any operations, error handling is correct, and the server pipe creation logic is sound.
58-66: LGTM!Straightforward async operation with proper resource handling and error propagation.
110-116: LGTM!Correctly uses the
checked_pathfrom the permission check for pipe creation, and properly registers the resource.
|
Is it also possible to add support for Pipe.prototyoe.open it would fix node pty and gemini #31032 |
Co-authored-by: Cyan Changes <contact@cyans.me>
Good idea, I added support to |
260455f to
29a9b1b
Compare
Sure, I squashed all commits and added him. |
While it's not wrong to specify an author for each commit, adding it to the PR summary is probably sufficient like below.👍 |
Oh, I didn't know that, thanks. I also add it in the PR description. |
bartlomieju
left a comment
There was a problem hiding this comment.
LGTM, I got a few nitpicky comments, but they are not worth holding this up
|
Thank you
…________________________________
From: Felipe Cardozo ***@***.***>
Sent: Friday, December 19, 2025 8:50:19 AM
To: denoland/deno ***@***.***>
Cc: Cyan ***@***.***>; Mention ***@***.***>
Subject: Re: [denoland/deno] feat: support named pipe listen, connect and open (PR #31624)
[https://avatars.githubusercontent.com/u/25258368?s=20&v=4]fraidev left a comment (denoland/deno#31624)<#31624 (comment)>
That's great. It's still under review, but would it be better to include @CyanChanges<https://github.com/CyanChanges> as a co-author?
Sure, I squashed all commits and added him.
—
Reply to this email directly, view it on GitHub<#31624 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AQLAIBBWU7U2MNPYU6SDPET4CM4TXAVCNFSM6AAAAACPEMNAT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMNZSG43TAMJTHE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Co-authored-by: Cyan Changes contact@cyans.me
Add support to Pipe.prototype.listen, Pipe.prototype.connect for windows with named pipes.
Also support to Pipe.prototype.open for unix.
Alternative of #29308, but without
Deno:pipe.Only with node compatibilities.
Solution for:
#25867
#28332
#31032
Maybe related:
#10244
I tested the Nuxt and Nx, they are working fine on Windows now.
I also tested node-pty, and it's working on Unix.