Skip to content
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

stdio: cannot write to stdin after #1233 #9206

Closed
bnoordhuis opened this issue Oct 20, 2016 · 13 comments
Closed

stdio: cannot write to stdin after #1233 #9206

bnoordhuis opened this issue Oct 20, 2016 · 13 comments
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. process Issues and PRs related to the process subsystem.

Comments

@bnoordhuis
Copy link
Member

Continuing from #9201 (comment). The change from #1233 makes it impossible to write to stdin, something that works in v0.10 and v0.12.

Test case:

var spawn = require('child_process').spawn;
var args = ['-e', 'process.stdin.write("ok\\n")'];
var proc = spawn(process.execPath, args, { stdio: ['pipe'] });
proc.stdin.pipe(process.stdout);

Trace:

$ strace -s1024 -fe write out/Release/node tmp/bug9201.js
...
[pid 25306] write(2, "events.js:160\n      throw er; // Unhandled 'error' event\n      ^\n\nError: write after end\n    at writeAfterEnd (_stream_writable.js:192:12)\n    at Socket.Writable.write (_stream_writable.js:243:5)\n    at Socket.write (net.js:661:40)\n    at [eval]:1:15\n    at ContextifyScript.Script.runInThisContext (vm.js:25:33)\n    at Object.exports.runInThisContext (vm.js:77:17)\n    at Object.<anonymous> ([eval]-wrapper:6:22)\n    at Module._compile (module.js:582:32)\n    at bootstrap_node.js:345:29\n    at _combinedTickCallback (internal/process/next_tick.js:67:7)\n", 554) = 554
...

cc @indutny

@bnoordhuis bnoordhuis added the process Issues and PRs related to the process subsystem. label Oct 20, 2016
@bnoordhuis bnoordhuis changed the title stdio: no longer possible to write to stdin after #1233 stdio: cannot write to stdin after #1233 Oct 20, 2016
@Fishrock123
Copy link
Contributor

sounds like a bug

@indutny
Copy link
Member

indutny commented Oct 23, 2016

@bnoordhuis does it actually work on windows? IMO, writing to stdin sounds quite ridiculous... It may not even support writes at all.

@Fishrock123
Copy link
Contributor

Are we actually talking about writing to fd 0, or about writing to the node stream representing it?

@bnoordhuis
Copy link
Member Author

The node stream. File descriptor 0 in the example accepts writes just fine, it's a bidirectional UNIX socket.

@Fishrock123
Copy link
Contributor

That's what I was thinking. I'm not sure if we care much about that odd Windows caveat then since I don't think it is possible to hit from this codepath anyways.

@indutny
Copy link
Member

indutny commented Oct 29, 2016

@bnoordhuis is it always such thing, though? Is it logical to allow writes to it?

@bnoordhuis
Copy link
Member Author

It's not typical but it's also not unheard of in the UNIX world. The current restriction is artificial. All other things being equal, it's better to lift it.

@Trott
Copy link
Member

Trott commented Jul 15, 2017

@bnoordhuis Should this remain open?

@bnoordhuis bnoordhuis added good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Jul 15, 2017
@bnoordhuis
Copy link
Member Author

Just tried it again, it's still unfixed. I've added 'good first contribution' and 'help wanted' labels.

@nikshepsvn
Copy link

Would love to try tackling this @bnoordhuis, this is my first open source contribution though so I would love some help with getting started. Can you give me an idea of what I should be looking at/doing to get this done?

@bnoordhuis
Copy link
Member Author

@nikshepsvn It was introduced in commit 9ae1a61 but that code has since migrated to lib/internal/process/stdio.js.

If you want a real quick test, echo | ./out/Release/node -e 'process.stdin.write(".")' should not throw an error.

FWIW, I'm also fine with closing this. I since learned that python also rejects writes to sys.stdin so at least we're in good company.

@JackDanger
Copy link

I second @bnoordhuis's recommendation to close this as Ruby and Python both open STDIN in read-only mode.

@TimothyGu TimothyGu removed the good first issue Issues that are suitable for first-time contributors. label Feb 5, 2018
@apapirovski
Copy link
Member

I'll close this out given the lack of activity in terms of making it actually happen and the latest feedback from @bnoordhuis and @JackDanger. If anyone still wants this though feel free to reopen or create an issue/PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants