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

[Process] Fixed data in pipe being truncated if not read before process termination #10153

Merged
merged 1 commit into from Mar 12, 2014

Conversation

Projects
None yet
4 participants
@astephens25
Copy link

commented Jan 28, 2014

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #9409
License MIT
Doc PR N/A

This is a repeat of the botched pull request #9630.

if (strlen($data) > 0) {
$read[$type] = $data;
while (strlen($data = fread($pipe, 8192)) > 0) {
isset($read[$type]) ? $read[$type] .= $data : $read[$type] = $data;

This comment has been minimized.

Copy link
@stloyd

stloyd Jan 28, 2014

Contributor

Instead of isset() in loop call:

$read[$type] = '';
while (strlen($data = fread($pipe, 8192)) > 0) {
    $read[$type] .= $data;
}

This comment has been minimized.

Copy link
@astephens25

astephens25 Jan 28, 2014

Author

That will ensure $read[$type] is set even if there is no data and will break other tests. The alternative would be to add another variable outside the loop. Look at the thread for #9630 as to why I did it this way. Thoughts @romainneutron?

This comment has been minimized.

Copy link
@stloyd

stloyd Jan 28, 2014

Contributor

Then after loop call:

if ('' === $read[$type]) {
    unset($read[$type]);
}

Calling isset() in loop, add not needed internal calls (especially for big output) that can be easily skipped.

This comment has been minimized.

Copy link
@astephens25

astephens25 Jan 28, 2014

Author

I would actually prefer:

$data = '';
while ($dataread = fread($pipe, 8192)) {
    $data .= $dataread;
}

if ($data) {
    $read[$type] = $data;
}

That way we are not reliant on ensuring something gets unset, but regardless, I created something more in line with the suggestions in #9630. Cheers.

This comment has been minimized.

Copy link
@romainneutron

romainneutron Mar 10, 2014

Member

Hey, sorry for the delay,

We need to fix this and I'd rather prefer the latest proposition @astephens25 provided

Could you update your PR ?

@astephens25

This comment has been minimized.

Copy link
Author

commented Mar 12, 2014

I updated the pull request with the suggestions from the previous diff.

@romainneutron

This comment has been minimized.

Copy link
Member

commented Mar 12, 2014

👍

fabpot added a commit that referenced this pull request Mar 12, 2014

bug #10153 [Process] Fixed data in pipe being truncated if not read b…
…efore process termination (astephens25)

This PR was merged into the 2.3 branch.

Discussion
----------

[Process] Fixed data in pipe being truncated if not read before process termination

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9409
| License       | MIT
| Doc PR        | N/A

This is a repeat of the botched pull request #9630.

Commits
-------

7e51913 Fixed data in pipe being truncated if not read before process termination

@fabpot fabpot merged commit 7e51913 into symfony:2.3 Mar 12, 2014

1 check passed

default Success: Travis, fabbot
Details
usleep(250000);
if ($p->isRunning()) {
$this->fail('Process execution did not complete in the required time frame');

This comment has been minimized.

Copy link
@romainneutron

romainneutron Mar 14, 2014

Member

Hello @astephens25, is there any reason in triggering a failure here ? Tests are failing quite often on travis (some systems are slower, sometimes the overhead of running a process is quite long.

Can I remove it safely ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.