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

piped commands don't work #5

Closed
vtjnash opened this issue Apr 1, 2012 · 13 comments
Closed

piped commands don't work #5

vtjnash opened this issue Apr 1, 2012 · 13 comments
Assignees

Comments

@vtjnash
Copy link
Collaborator

vtjnash commented Apr 1, 2012

A simple example, where the input of the next depends on a previous command, like run(echo a | cat -b -) hangs forever

@Keno
Copy link
Owner

Keno commented Apr 1, 2012

Ah yes, libuv is designed to be used by Node and strangly enough piping isn't supported the way I'd like it to be. It's a relatively small patch for windows and I'll do the same for unix.

@Keno
Copy link
Owner

Keno commented Apr 1, 2012

basic pipes work now. I'll do the more complex stuff tomorrow.

@Keno Keno closed this as completed Apr 1, 2012
@vtjnash
Copy link
Collaborator Author

vtjnash commented Apr 1, 2012

It doesn't hang anymore, which is good, but the pipes appear to be all connected to stdin/stdout instead of end to end.

@Keno
Copy link
Owner

Keno commented Apr 1, 2012

Hmm that's odd I'll look into it

@Keno Keno reopened this Apr 1, 2012
@ghost ghost assigned Keno Apr 1, 2012
@vtjnash
Copy link
Collaborator Author

vtjnash commented Apr 18, 2012

issue summary: uv_spawn requires access to both ends of the stdin and stdout pipes. broken testcase is readall(echo a & echo b)

[RFC] resolution proposal:
Should we (A) explicitly split fd into fd_read and fd_write (or fd[2]) everywhere or (B) add a field fd_end2 that is used just during process spawning? I'm willing to work on writing either.

Pro/Cons of these that I can see currently are:
Proposal A provides the most consistent interface across libuv since it is very explicit. It is also a bit annoying for the current wide range of locations this doesn't matter. It impacts the most code too, which could make it difficult to not introduce additional bugs and harder to merge. But if this is possibly useful for the future of libuv, that isn't so terrible.

Proposal B is simple and relatively contained, although it also introduces some potentially unexpected behavior if someone manages to first open the fd in some other way. References to uv__close(fd) will need to also need to close both ends, but it won't be as easy to find these references as it would if we introduce an complete incompatibility, such as proposal A. However, it does maintain backwards compatibility if anyone was already accessing fd directly.

Am I forgetting anything important? Eventually, we could elicit feedback from from the libuv maintainers too. However, I wanted to hammer out the details a bit first here.

@isaacs
Copy link

isaacs commented Apr 22, 2012

The libuv child_process stdio is in the middle of being refactored. Feedback would be most appreciated.

Here's the tracking issue in node: nodejs/node-v0.x-archive#3065

@Keno
Copy link
Owner

Keno commented May 28, 2012

The upstream refactor is done.

@vtjnash
Copy link
Collaborator Author

vtjnash commented May 30, 2012

They haven't implemented the proposed uv_pipe (which wouldn't be exactly ideal anyways) or reuse of pipes, so it is still not possible to fix with unmodified libuv. It is now possible to give raw file descriptors, so it is now possible to do file IO pipes.

@vtjnash
Copy link
Collaborator Author

vtjnash commented Jun 10, 2012

basic stuff in julia refactor now works (pull libuv from https://github.com/vtjnash/libuv, pipes branch, if you are trying to build on linux right now)

@Keno
Copy link
Owner

Keno commented Jun 10, 2012

Awesome. Does you branch of libuv pass the libuv unit tests?

@vtjnash
Copy link
Collaborator Author

vtjnash commented Jun 10, 2012

yes, although it might be (probably is?) not fully complete. i was hoping you would take a look before merging it with the pull request

@Keno
Copy link
Owner

Keno commented Jun 10, 2012

Will do. I'm flying back to Europe for the summer tomorrow, so I won't get to it until Tuesday though (unless I get to it while on the plane).

@vtjnash
Copy link
Collaborator Author

vtjnash commented Jul 6, 2012

closing this for now as pipe commands pretty much work and I am tracking the missing functionality at the top of stream.jl as I go

@vtjnash vtjnash closed this as completed Jul 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants