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

Issue with getIsWindows() change. #9

Closed
epicwally opened this issue Nov 2, 2015 · 5 comments
Closed

Issue with getIsWindows() change. #9

epicwally opened this issue Nov 2, 2015 · 5 comments

Comments

@epicwally
Copy link

The recent added check in the $descriptors array for windows has broken functionality for me. Running on a linux machine. Previously had the 'a' descriptor, but now with the getIsWindows() check on line 286, it's returning false, giving a 'w' descriptor. If I am building a small pdf this doesn't seem to be a problem, but for larger pdfs it just hangs. I have temporarily commented out the change here returning it to using the 'a', and everything seems to be in order again.

@mikehaertl
Copy link
Owner

Hmm, please have a look at issue #6 for why this was changed. I don't have much time currently to look into this. But maybe you can do some research and find a fix that makes everyone happy?

@mkloosterman
Copy link

It seems that there are 2 issues here. When proc_open is configured with the stderr descriptor as:
2 => array('pipe','w')
there is a deadlock condition for generating large PDF's. But when the workaround:
2 => array('pipe','a')
is applied, the error reporting breaks on Windows.

Here is a fix that should address both issues. Open the stderr stream in write mode (which will keep the error reporting working) but then set both streams to non-blocking mode. I have tested it on Linux and it addresses the deadlock issue. The error reporting still works fine.

$descriptors = array(
    1 => array('pipe','w'),
    2 => array('pipe','w')
);
$process = proc_open($command, $descriptors, $pipes, $this->procCwd, $this->procEnv, $this->procOptions);

if (is_resource($process)) {
    // set streams to non blocking mode
    stream_set_blocking($pipes[1], 0);
    stream_set_blocking($pipes[2], 0);
    $this->_stdOut = trim(stream_get_contents($pipes[1]));
    $this->_stdErr = trim(stream_get_contents($pipes[2]));
    fclose($pipes[1]);
    fclose($pipes[2]);
...

@mikehaertl
Copy link
Owner

@epicwally @mkloosterman I'm hesitant to change this. I remember having messed around with this a lot. And I think we even already tried the stream_set_blocking() at some point - which opened other issues again. See here:

mikehaertl/phpwkhtmltopdf#78 (comment)

So this still needs more research. I'm open for any input, though.

@mikehaertl
Copy link
Owner

@epicwally Does it help if you keep the pipe configuration as it is and only add the stream_set_blocking() parts that @mkloosterman suggests?

@mikehaertl
Copy link
Owner

mikehaertl commented May 20, 2016

Closing this, as there was no further feedback.

I'm more than willing to improve pipe/blocking configuration, but it must be handled with care and take all error reports into account. ~~~So feel free to comment on #6 hich I would reopen if we find a better setup.~~~ Update: I try keep #20 updated with our latest findings.

This issue was closed.
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

No branches or pull requests

3 participants