Windows fix

[moufmouf]

Another Windows related pull-request.

I tried to fix the bugs that are related to STDOUT containing about 4096 instead of 10000 and Windows hanging for 32 seconds before returning a 100000 charcters output.
To circumvent the STDOUT / STDERR problem, I redirect the output and error of the program to a file.
This file is read by a stream. Of course, this is a Windows only workaround.

This works great, but has a security impact, since stdout and stderr are written to the disk. It is also an issue for long running processes, since those could fill the hard-drive fairly quickly.
Files are automatically deleted at the end of the process, but of course, things can go wrong in which case file won't be deleted.

So this is definitely not the perfect workaround, but at least, it makes child-process usable in Windows while waiting a fix in PHP, if it ever comes (the issue has been open for quite a few years).

What do you think about this?

[Beanow]

Writing to the filesystem will have a great performance impact as well. If you have any control over the target program I would switch to some form of IPC over a file stream.

For example, one option for windows is to sidestep the PHP bug by spawing a mediating application (in a different language) that does nothing but pipe these outputs to TCP. Or change the eventual child-process to do this.

So what I would do is make this an explicit option. Using the STDERR and STDOUT channels by default in the hopes that your child-process won't actually get past 4096 bytes of output.

[moufmouf]

Hi Robin,

Thanks for your feedback, it makes sense to add it as an option (so that if the bug is ever fixed in PHP, the user can decide to remove the tweak). I just added a useWindowsWorkaround method.

$process->useWindowsWorkaround(true);

I'm not sure if the naming is perfect, if you have a better idea, let me know. Also, I added a more complete documentation.

[jmikola]

Same comment as https://github.com/reactphp/child-process/pull/10/files#r25787513. Not necessary, IMO.

[Beanow]

The only issue I have left is I think the name should be based on the kind of workaround rather than the platform. So something like: useFilestreams and startFilestreams. Skipping the check for windows version entirely (defined('PHP_WINDOWS_VERSION_BUILD')).

This way the workaround can be applied independently of platform, but rather for any environment that benefits from this. Since you now have to enable it explicitly I don't see this causing problems for anyone.

[moufmouf]

Hi Robin,

I understand your comment. You want the workaround to be applicable whatever the platform used is. This is a good idea. This will in particular increase testability on Unix systems. However, I find this will be harder for everyday users to write code that works on both platforms because each user will have to write:

$process = new Process(...);
if (defined('PHP_WINDOWS_VERSION_BUILD')) {
    $process->useWindowsWorkaround(true);
}

This is long and I feel that "by default", the version that "just works" should be used.

So here is another idea of refactoring.

I could revert all my work on the Process class. Instead, I write a FileStreamProcess class that extends the Process class with special Windows workaround.
Finally, I write a ProcessFactory class with create method that will create either a Process or a FileStreamProcess with all the best options to use depending on the platform.
Would that be ok with you?

[Beanow]

Yes I think that's a good idea. This will still allow users of any platform to make an explicit choice by not using the factory.

[clue]

Thanks for this PR @moufmouf! 👍

This PR improves compatibility significantly (#9), very much appreciated!

For the reference, this appears to essentially implement what is also present in Symfony: 1, 2, 3

[clue]

Not sure if the analysis in #9 also applies to STDIN. Assuming it does, this means that we will still block when writing more data to the STDIN pipe as there's no way to detect when the receiving buffer is full?

[WyriHaximus]

Without going to code details of this PR right now I'de like to confirm that this fixes reading from STDIN, and writing to STDOUT, STDERR from a child process. Good work @moufmouf I'll dig into the code specifics later :).

[moufmouf]

Hi @WyriHaximus ,

Yes, in Windows, there is an issue if more than 4096 is sent. This fixes the problem.
Actually, I ran into another problem with the proposed solution where in some cases, Windows messes the order of the messages sent back (the STDOUT stream is messed up).

This was almost a year ago and I must admit I don't really remember the details. I remember that this fix improves a lot the compatibility with Windows, but is not the ultimate solution.

Also, I haven't had a chance to test this with PHP 7. Maybe the solution would be to simply discard the PR if PHP 7 fixes the Windows compatibility issue.

[WyriHaximus]

Just installed PHP 7 on my Windows 10 box so I can look into that. Funny thing is this fixed it for me with very small messages. Anyway to trigger the STDOUT order issue? Or just push massive data over it?

[clue]

Thank you for digging into this and filing this PR! 👍

I still think that getting this feature in makes perfect sense and I'm looking forward to a future update here as per #9 (comment)!

Given that this is currently a WIP and hasn't seen any updates in a while and I don't see it's likely this will get traction any time soon, I will have to close this for now. Please don't see this as discouraging, we would still love to get this feature in and very much appreciate your effort! You can simply complete this feature and update this PR and we're happy to reopen this 👍

Thank you for your effort!