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

Make run() fully non-blocking and fix potential other problems #1403

Merged
merged 1 commit into from Jun 22, 2011
Merged

Make run() fully non-blocking and fix potential other problems #1403

merged 1 commit into from Jun 22, 2011

Conversation

lenar
Copy link
Contributor

@lenar lenar commented Jun 22, 2011

Multiple changes:

  1. make writing to process non-blocking too - otherwise there might be increased possibility for buffer deadlock
    given big enough input data. Also now it's guaranteed that all stdin data will be written.

  2. get rid of fgets() - fgets() isn't really good function to use in case of non-blocking sockets. Data loss possible.

fabpot added a commit that referenced this pull request Jun 22, 2011
Commits
-------

bda4129 Make run() fully non-blocking and fix potential other problems

Discussion
----------

Make run() fully non-blocking and fix potential other problems

Multiple changes:

1) make writing to process non-blocking too - otherwise there might be increased possibility for buffer deadlock
given big enough input data. Also now it's guaranteed that all stdin data will be written.

2) get rid of fgets() - fgets() isn't really good function to use in case of non-blocking sockets. Data loss possible.
@fabpot fabpot merged commit bda4129 into symfony:master Jun 22, 2011
@fabpot
Copy link
Member

fabpot commented Jun 22, 2011

Does it make #1365 obsolete?

@lenar
Copy link
Contributor Author

lenar commented Jun 22, 2011

@fabpot: After reading, I really don't know. Let's hope. But ...

I now improved Process tests a bit to test stdout, stderr with different stream sizes and different
behaviours of child processes. Added it to non-blocking-process branch, commit 2d29a82.
In my case, nothing fails, but maybe this helps other people. Or Windows people - I myself cannot test on Windows.

@fabpot
Copy link
Member

fabpot commented Jun 23, 2011

These tests pass on my Linux box but fail on my Mac.

@fabpot
Copy link
Member

fabpot commented Jun 23, 2011

Actually, on the Mac, the tests behave correctly but the exit code is -1 instead of 0.

@lenar
Copy link
Contributor Author

lenar commented Jun 23, 2011

Could you check if the $this->status['running'](after call to proc_get_status%28%29) is true in the case you get -1.

On my linux I got it -1 couple of times. 99% of time it doesn't happen. I theorized it's because sometimes the child
process isn't finished enough and finally I got confirmation too that in case of -1 the process is still running (stats['running'] === true).

But it's really almost unreproducible on my Linux. So if you have this value every time it might be easier for you to find solution.

What comes into my mind:

  1. maybe we should poll, let's say if process is still running we usleep(1000) and the try proc_get_status() again until not running. Maybe up to a 1 sec.

  2. maybe, if the process is still running we can trust the return value subsequently given by proc_close()?

Or maybe there's some other problem on Mac.

fabpot added a commit that referenced this pull request Jun 23, 2011
Commits
-------

2d29a82 New test for Process, testing stdout and stderr at different stream sizes

Discussion
----------

Make run() fully non-blocking and fix potential other problems

Multiple changes:

1) make writing to process non-blocking too - otherwise there might be increased possibility for buffer deadlock
given big enough input data. Also now it's guaranteed that all stdin data will be written.

2) get rid of fgets() - fgets() isn't really good function to use in case of non-blocking sockets. Data loss possible.

---------------------------------------------------------------------------

by fabpot at 2011/06/22 07:11:55 -0700

Does it make #1365 obsolete?

---------------------------------------------------------------------------

by lenar at 2011/06/22 14:08:14 -0700

@fabpot: After reading, I really don't know. Let's hope. But ...

I now improved Process tests a bit to test stdout, stderr with different stream sizes and different
behaviours of child processes. Added it to non-blocking-process branch, commit 2d29a82.
In my case, nothing fails, but maybe this helps other people. Or Windows people - I myself cannot test on Windows.

---------------------------------------------------------------------------

by fabpot at 2011/06/22 22:59:55 -0700

These tests pass on my Linux box but fail on my Mac.

---------------------------------------------------------------------------

by fabpot at 2011/06/22 23:05:14 -0700

Actually, on the Mac, the tests behave correctly but the exit code is `-1` instead of `0`.

---------------------------------------------------------------------------

by lenar at 2011/06/23 01:23:51 -0700

Could you check if the $this->status['running'] (after call to proc_get_status()) is true in the case you get -1.

On my linux I got it -1 couple of times. 99% of time it doesn't happen. I theorized it's because sometimes the child
process isn't finished enough and finally I got confirmation too that in case of -1 the process is still running (stats['running'] === true).

But it's really almost unreproducible on my Linux. So if you have this value every time it might be easier for you to find solution.

What comes into my mind:

1) maybe we should poll, let's say if process is still running we usleep(1000) and the try proc_get_status() again until not running. Maybe up to a 1 sec.

2) maybe, if the process is still running we can trust the return value subsequently given by proc_close()?

Or maybe there's some other problem on Mac.
@fabpot
Copy link
Member

fabpot commented Jun 23, 2011

Thanks a lot for the info @lenar. You are right, the process is still running, and that's why we get a -1. I've just pushed a patch that makes everything work on my Mac:

100a8dc

I will test on Windows now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants