Skip to content

[Process] Redirect output without a shell #9726

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

Closed
wants to merge 2 commits into from
Closed

Conversation

vektah
Copy link

@vektah vektah commented Dec 9, 2013

Currently processes that output large amounts of data will block. If
the output is not important then this becomes an issue. It can be worked
around by redirecting the output to > /dev/null but this requires an
instance of /bin/sh to do the work.

This patch adds the ability to set the processPipes, and a new
processPipe that redirects to /dev/null.

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #9007
License MIT
Doc PR symfony/symfony-docs#3303

Currently processes that output large amounts of data will block. If
the output is not important then this becomes an issue. It can be worked
around by redirecting the output to > /dev/null but this requires an
instance of /bin/sh to do the work.

This patch adds the ability to set the processPipes, and a new
processPipe that redirects to /dev/null.

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#9007
| License       | MIT
| Doc PR        | symfony/symfony-docs#3303
@stof
Copy link
Member

stof commented Dec 9, 2013

This patch currently allows to change the ProcessPipes at any time. But this will break if you change it after the process is started. A safeguard should be added to prevent it.
Another issue is that the ProcessPipes instance is not replaced anymore when stopping a process and starting it again with your new logic, while it is replaced with the current logic. I don't know if it can introduce some weird bugs though.

And I think this should be added to the ProcessBuilder as well

);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra line break

@vektah
Copy link
Author

vektah commented Dec 9, 2013

@stof

Another issue is that the ProcessPipes instance is not replaced anymore when stopping a process and starting it again with your new logic, while it is replaced with the current logic. I don't know if it can introduce some weird bugs though.

I'm not sure what weirdness is expected here, I've added a test...

@fabpot
Copy link
Member

fabpot commented Dec 11, 2013

ping @romainneutron

@romainneutron
Copy link
Contributor

I started to work on the same ticket few weeks ago, here's my implementation (missing doc blocks and more tests, not yet finished)

As far as I remember, this implementation has issue on windows platform and mine too. I've planned to check this on saturday hackday in SymfonyCon, will you be there @vektah ?

@vektah
Copy link
Author

vektah commented Dec 11, 2013

No I wont =(

That seems like a fairly solid fix for this bug, though it wouldn't allow for other custom pipes - ie it might be possible to redirect one process's output into anothers input, again without a shell...

Will $nullstream = fopen(defined('PHP_WINDOWS_VERSION_BUILD') ? 'NUL' : '/dev/null', 'c'); leave dangling file handles? I don't see an fclose...

@romainneutron
Copy link
Contributor

Yep, we could think of a Process::pipe method ;)

Passed streams should be fclosed the same way they arealready close here

About both implementation we have, outside the troubles it could bring on Windows (I'm gonna test it this week end) I've to admit I'd rather prefer easy natural method name on the Process class rather than manipulate ProcessPipes or extended classes. What do you think ?

@vektah
Copy link
Author

vektah commented Dec 12, 2013

Why not both?

disableOutput() is a nicer interface

Set processPipes leaves the class open for extension but closed to modification. It's a pain when you need a feature not to be able to add it without changing base classes.

disableOutput could internally setProcessPipes(new NullPipes())

@stof
Copy link
Member

stof commented Dec 12, 2013

disableOutput() seems a better fit for the ProcessBuilder IMO

@vektah
Copy link
Author

vektah commented Dec 13, 2013

The other nice thing you could do with setProcessPipes is have a passthrough, binding to STDIN, STDOUT and STDERR.

@fabpot
Copy link
Member

fabpot commented Mar 12, 2014

closed in favor of #10425

@fabpot fabpot closed this Mar 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants