-
Notifications
You must be signed in to change notification settings - Fork 5.1k
child_process: make piped stdio streams instances of net.Socket #36316
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
012f1ff
96d0dab
7ac0f5e
071d3fb
9d213dd
c6a39ad
5299c8a
f13a010
d9649f6
e7ce0d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -384,10 +384,10 @@ | |
| if (fd == null) { | ||
| this[kFs] = customFs || fs; | ||
| this.fd = null; | ||
| // Internal $fastPath callers (writableFromFileSink) discard .path; do not | ||
| // resolve it - path.resolve("") needs process.cwd(), which throws when | ||
| // the cwd has been deleted (Node still spawns children in that state). | ||
| // Internal $fastPath callers discard .path; do not resolve it - | ||
| // path.resolve("") needs process.cwd(), which throws when the cwd has | ||
| // been deleted (Node still spawns children in that state). | ||
| this.path = fastPath ? path : getValidatedPath(path); | ||
|
Check warning on line 390 in src/js/internal/fs/streams.ts
|
||
|
Comment on lines
+387
to
390
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Removing Extended reasoning...What the finding isThis PR removes // removed by this PR
const w = new WriteStream("", { $fastPath: true }); // no fd → takes the fd == null branchThe two remaining
Both take the The specific dead codeif (fd == null) {
this[kFs] = customFs || fs;
this.fd = null;
// Internal $fastPath callers discard .path; do not resolve it -
// path.resolve("") needs process.cwd(), which throws when the cwd has
// been deleted (Node still spawns children in that state).
this.path = fastPath ? path : getValidatedPath(path);
...The ternary at line 390 now always evaluates to Note that Step-by-step proof
Why it should change in this PRREVIEW.md, Code style & idioms: "Delete dead code in the same PR that makes it dead." This PR's own diff (the This is also the root-cause fix for the still-open comment-cop finding on streams.ts:389 ("If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong"): the workaround the comment justifies has no caller, so the right fix is deletion, not shortening. ImpactZero functional impact — the ternary's dead arm is never taken, so behavior is unchanged. This is a nit: harmless but directly created by (and touched in) this PR. FixReplace lines 387–390 with: this.path = getValidatedPath(path); |
||
| const { flags, mode } = options; | ||
| this.flags = flags === undefined ? "w" : flags; | ||
| this.mode = mode === undefined ? 0o666 : mode; | ||
|
|
@@ -796,20 +796,8 @@ | |
| } | ||
| } | ||
|
|
||
| function writableFromFileSink(fileSink: any) { | ||
| $assert(typeof fileSink === "object", "fileSink is not an object"); | ||
| $assert(typeof fileSink.write === "function", "fileSink.write is not a function"); | ||
| $assert(typeof fileSink.end === "function", "fileSink.end is not a function"); | ||
| const w = new WriteStream("", { $fastPath: true }); | ||
| $assert(w[kWriteStreamFastPath] === true, "fast path not enabled"); | ||
| w[kWriteStreamFastPath] = fileSink; | ||
| w.path = undefined; | ||
| return w; | ||
| } | ||
|
|
||
| export default { | ||
| ReadStream, | ||
| WriteStream, | ||
| kWriteStreamFastPath, | ||
| writableFromFileSink, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,12 +53,12 @@ | |
|
|
||
| let debugId = 0; | ||
|
|
||
| function constructNativeReadable(readableStream: ReadableStream, options): NativeReadable { | ||
| function constructNativeReadable(readableStream: ReadableStream, options, Base?): NativeReadable { | ||
| $assert(typeof readableStream === "object" && readableStream instanceof ReadableStream, "Invalid readable stream"); | ||
| const bunNativePtr = (readableStream as any).$bunNativePtr; | ||
| $assert(typeof bunNativePtr === "object", "Invalid native ptr"); | ||
|
|
||
| const stream = new Readable(options); | ||
| const stream = Base !== undefined ? new Base(options) : new Readable(options); | ||
|
robobun marked this conversation as resolved.
|
||
| stream._read = read; | ||
| stream._destroy = destroy; | ||
|
|
||
|
|
@@ -260,20 +260,20 @@ | |
| } | ||
| } | ||
|
|
||
| function ref(this: NativeReadable) { | ||
| const ptr = this.$bunNativePtr; | ||
| if (ptr === undefined) return; | ||
| if (this[kRefCount]++ === 0) { | ||
| if (ptr !== undefined && this[kRefCount]++ === 0) { | ||
| ptr.updateRef(true); | ||
| } | ||
| return this; | ||
| } | ||
|
Check warning on line 269 in src/js/internal/streams/native-readable.ts
|
||
|
Comment on lines
263
to
269
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Extended reasoning...What the bug is
function ref(this: NativeReadable) {
const ptr = this.$bunNativePtr;
if (ptr !== undefined && this[kRefCount]++ === 0) {
ptr.updateRef(true);
}
return this;
}The post-increment has no upper cap, so a redundant Code path / step-by-step proofFor a live
Result: after Why existing code doesn't prevent itThe 5299c8a fix only touched the decrement side. Nothing caps the increment. And the fully-idempotent Internal inconsistencyWithin this PR's own diff, the three stdio streams now disagree on ref/unref semantics: Reproconst { spawn } = require('child_process');
const c = spawn(process.execPath, ['-e', 'setTimeout(()=>{},1e6)'], { stdio: 'pipe' });
c.stdin.ref(); c.stdin.unref(); // stdin: unrefed (flag path)
c.stdout.ref(); c.stdout.unref(); // stdout: still refed (counter went 1→2→1)Impact / severityNit. Triggering it requires a redundant FixCap function ref(this: NativeReadable) {
const ptr = this.$bunNativePtr;
if (ptr !== undefined && this[kRefCount] === 0) {
this[kRefCount] = 1;
ptr.updateRef(true);
}
return this;
}Or drop the instance-level |
||
|
|
||
| function unref(this: NativeReadable) { | ||
| const ptr = this.$bunNativePtr; | ||
| if (ptr === undefined) return; | ||
| if (this[kRefCount]-- === 1) { | ||
| if (ptr !== undefined && this[kRefCount] > 0 && --this[kRefCount] === 0) { | ||
| ptr.updateRef(false); | ||
| } | ||
| return this; | ||
| } | ||
|
robobun marked this conversation as resolved.
|
||
|
|
||
| export default { constructNativeReadable }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code