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

Windows compatibility? #9

Closed
moufmouf opened this issue Feb 27, 2015 · 18 comments · Fixed by #67
Closed

Windows compatibility? #9

moufmouf opened this issue Feb 27, 2015 · 18 comments · Fixed by #67

Comments

@moufmouf
Copy link

Hi guys,

I've been trying to use reactphp/child-process on Windows and I've had hard times having it to work.
I traced the problem back to a bug in PHP:

https://bugs.php.net/bug.php?id=51800

The bug has been solved, at least in recent PHP versions.

To sum it up:

I have an application that uses child-process and that works flawlessly on Linux.
On Windows, reactphp/child-process does not work with PHP 5.5.12. The PHP process stalls and nothing is happening (just like in the PHP bug)

It seems to work with PHP 5.6.6, although I'm having some strange behaviour (I'm facing locks of several seconds, then the application starts again...)

I've tried to run the unit tests of the master branch and many are failing in Windows. I'm not sure if those are supposed to work. Did you have any of those unit tests running on Windows? If yes, what version of PHP did you use?

Do you need some help working on that compatibility or is there something I missed?

@clue
Copy link
Member

clue commented Apr 13, 2015

I've been trying to use reactphp/child-process on Windows and I've had hard times having it to work.

Same here, unfortunately I'm having to give up on this for now..
I've got this to work to some degree, however finding some test cases that still fail wasn't too difficult.
(I'll try to file a PR to provide these in the upcoming days)

Current status (afaict):

  • PR Windows test suite compatibility #13 (originally Fixing unit tests for Windows #10) aims to improve the test suite for Windows
  • PR Windows fix #11 aims to work around Windows' limitations by using file handles instead of process pipes.
    This is a significant improvement, however, this also introduces several other issues (writing to disk, different buffering behavior, possible privacy leak, degraded performance, stale files etc.)
  • I've looked into using stream_socket_pair() to no avail (works just fine on non-Windows)
  • I've looked into using a TCP/IP client/server sockets to no avail (works just fine on non-Windows)
  • I've looked into using names pipes to no avail

Has anybody ever reached a state in which this can be considered "compatible"? If so, which version / OS?

@clue
Copy link
Member

clue commented Apr 13, 2015

Afaict the compatibility issues are cause by two independent issues:

stream_set_blocking() does not work with pipes opened with proc_open() (https://bugs.php.net/bug.php?id=47918). This appears to be because Windows' process pipes do not support async operation, so this is unlikely to caused (or ever fixed) by PHP.

stream_select() (and family) always reports the pipes as readable even when no data is available. This will cause the Stream class to always invoke a fread() on the pipes.

  • Adding the above blocking stream behavior, this will thus block the event look.
  • This appears to be addressed in several commits in different PHP versions (not sure which exactly).

I could not reproduce any blocking behavior on proc_open() itself (https://bugs.php.net/bug.php?id=51800). I'm not sure if this issue ever did exist in the first place or if this could boil down to the above issue in fact.

@clue
Copy link
Member

clue commented Apr 21, 2015

  • PR Windows fix #11 aims to work around Windows' limitations by using file handles instead of process pipes.
    This is a significant improvement, however, this also introduces several other issues (writing to disk, different buffering behavior, possible privacy leak, degraded performance, stale files etc.)

I've looked into using stream_socket_pair() to no avail (works just fine on non-Windows)

Potentially, connected sockets could work around these limitations. Has anybody else tried using connected sockets instead of file descriptors?

FWIW: Here's my attempt (which does not work on Windows, but does work on non-Windows systems): https://github.com/reactphp/child-process/compare/master...clue-labs:sockets?expand=1

@joshdifabio
Copy link

  • I've looked into using names pipes to no avail – @clue

How far did you get with that? Did you find a way to create a named pipe on Windows?

@clue
Copy link
Member

clue commented Jun 12, 2015

  • I've looked into using names pipes to no avail – @clue

How far did you get with that? Did you find a way to create a named pipe on Windows? – @joshdifabio

Unfortunately not very far at all, I've toyed around with this idea to no avail.

@clue
Copy link
Member

clue commented Jul 7, 2016

stream_set_blocking() does not work with pipes opened with proc_open() (https://bugs.php.net/bug.php?id=47918). This appears to be because Windows' process pipes do not support async operation, so this is unlikely to caused (or ever fixed) by PHP.

See also my suggested PR reactphp/stream#46

With this change applied, the Process class would throw a RuntimeException on Windows, because it can not set the streams to be non-blocking.

IMO this does make sense, because we can't guarantee proper execution otherwise anyway.

Once this change is in, we should look into adding an option to start child processes without any STDOUT/STDIN streams, which sometimes makes sense on all platforms and should also work on Windows.

@kelunik
Copy link

kelunik commented Feb 9, 2017

The hardest thing is probably getting the correct exit code on Windows. It just doesn't seem to work.

@clue
Copy link
Member

clue commented Mar 8, 2017

@kelunik What issue are you running in to, can you provide a gist to reproduce this?

@clue
Copy link
Member

clue commented Mar 8, 2017

Let's recap what works and what doesn't:

  • Executing child processes ✔️
  • Writing to STDIN and reading from STDOUT: might block ❌
  • Returning exit-code: unknown ❓

@kelunik
Copy link

kelunik commented Mar 8, 2017

@clue We solved the exit code issue now, but it's not in master yet, using proc_get_status and a third pipe.

@clue
Copy link
Member

clue commented Mar 8, 2017

See also my suggested PR reactphp/stream#46

With this change applied, the Process class would throw a RuntimeException on Windows, because it can not set the streams to be non-blocking.

We MAY want to provide some means to start child processes without accessing its STDIO, in which case everything should work just fine (this can be useful for all systems).

We MAY then want to look into incorporating (or documenting) using a process wrapper such as https://github.com/cubiclesoft/createprocess-windows in order to redirect the STDIO streams through a TCP/IP connection, which would be mostly useful for Windows. This likely depends on the above feature first, so we can launch the wrapper script without accessing its STDIO streams at all.

@cubiclesoft
Copy link

I just happened to notice two stars appear on the 'createprocess-windows' project and that a lead dev for reactphp had starred it. Nice. At any rate, a PECL extension that specifically integrates the logic from the createprocess-windows project would be less heavy-handed. Starting a process is a particularly expensive system call on Windows, which makes starting two processes to start one process to avoid a known bug in PHP kind of overkill (PHP bug 47918).

A few thoughts I had: If you are sending stdin data, some processes might handle some data, then write to stdout/stderr, then read more data, write more, etc. A prematurely closed stdout/stderr pipe could result in premature process termination with problems like hard-to-diagnose "broken pipe" errors. But, as you already know, leaving the stdout/stderr pipes open results in blocking issues on Windows. Microsoft most likely left the issue as-is partially for legacy reasons as the Internet didn't exist at the time they created the Windows OS, but also because the Windows OS can only start about 20-50 processes simultaneously (depends on the hardware) before significant slowdowns start happening and the system spends most of its time starting processes instead of doing useful work. When it comes to process creation, Windows is not like *NIX architecture, the latter uses copy-on-write fork()/exec() to handle startup of new processes. As a result, *NIX can start 2 to 3 times as many processes simultaneously. There's still a hard limit on *NIX systems, but it's not as bad (e.g. the default precompiled Apache prefork limit is 256). The Windows solution for application scalability is to use a handful of threads and I/O Completion Ports instead of starting lots of processes and/or threads.

If you want to see example, working PHP code that uses the TCP/IP socket bits of createprocess.exe, I used it here:

https://github.com/cubiclesoft/cloud-storage-server-ext-scripts

See server_exts/scripts.php.

@clue
Copy link
Member

clue commented Mar 10, 2017

@cubiclesoft Thanks for the useful background information!

This library is essentially a low-level building block for many higher-level applications and Windows isn't exactly the main target platform for most consumers here.

Hence my suggestion that we should progress with reactphp/stream#46 first, which means that the current default logic now throws an Exception on Windows, so that we can no longer start any processes with STDIO attached on Windows at all. This makes sense because we can't use non-blocking streams here and this would otherwise cause all kinds of strange issues on Windows.

We should then add a way to start processes without attaching to STDIO at all, which can be useful for any platform AND should also work just fine on Windows.

We should then add a big fat warning about this behavior and suggest people use createprocess-windows if they really need to access STDIO on Windows (which I suppose many people do NOT need).

@clue
Copy link
Member

clue commented Jan 13, 2018

Hence my suggestion that we should progress with reactphp/stream#46 first, which means that the current default logic now throws an Exception on Windows, so that we can no longer start any processes with STDIO attached on Windows at all. This makes sense because we can't use non-blocking streams here and this would otherwise cause all kinds of strange issues on Windows.

Small progress update: We've introduced the above patch with #38 and accordingly have had to remove Windows support with #41/#47 in the v0.5.0 release. As an alternative, we currently recommend using Windows Subsystem for Linux (WSL) instead.

We are still looking into providing compatibility with Windows in the upcoming versions as discussed in this ticket.

With the upcoming v0.6.0 release, we will add a way to explicitly configure process pipes. This should also allows spawning a process without STDIO pipes on Windows. STDIO redirection (files, network or the above "createprocess" wrapper) can then be used to work around this limitation.

We will make sure to keep this ticket updated as we progress on this matter.

@kelunik
Copy link

kelunik commented Jan 13, 2018

I think STDIO redirection should be part of this package then and not be left to the user. Unlike unix sockets and pipes, network sockets do not have any means of built-in authentication or access protection.

https://github.com/amphp/windows-process-wrapper provides a wrapper with that in mind. It exchanges security tokens on the sockets to ensure there's no unauthorized information leakage by another process connecting first to one of the sockets. This handshake has a slight overhead, but this shouldn't really matter. If you need a lot of quickly running child processes, it might be noticeable, but then you're probably better of creating a PHP extension with bindings.

@cubiclesoft You might want to add a similar mechanism to your wrapper.

@cubiclesoft
Copy link

I've implemented a couple of options. I did realize at the time that the feature introduced a vulnerability but I figured that if this is an issue, then the system in question probably has far bigger problems that should be addressed first. The server portion is generally started attached to a localhost-only environment and the process runs in the same environment. Also, timing is a significant issue. All of that limits the potential attack surface. Not saying there isn't an issue here but it's a fairly minor vulnerability.

Of course with tokens I have to trust users to generate them correctly and securely or it defeats the purpose....

@resistancelion
Copy link

resistancelion commented Dec 26, 2019

But... You can use a most common way to solve the problem - workaround. so, what can we do?

  1. We can use Windows API (unrecommended, because it's requires unsafe php settings, or extensions)
  2. We can use temporary or virtual files to write and read from them. but...
    In php is not possible to open the same file/use same file asynchronous, so, instead of opening file two times i've used other functions for reading file.
    Example:

`<?php
while( true )
{
usleep(120);
if( strlen(($f = file_get_contents("C:/Users/Lev/Appdata/lws/temp.buff")) > 0)
{
file_put_contents("C:/Users/Lev/Appdata/lws/temp.buff", "");
echo PHP_EOL, "Process output:", PHP_EOL, $f, PHP_EOL;
}
}

UPD: sry, just finded it by Github's suggestion, and have no clue about problem status.

@clue
Copy link
Member

clue commented Dec 27, 2019

@levzenin Windows is supported, but the support is limited. We've included details in the documentation, including known work-arounds, such as using file handles to access the process output: https://github.com/reactphp/child-process#windows-compatibility

If you feel anything is unclear or missing, please file a new ticket and I'm happy to take a look at this again 👍

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

Successfully merging a pull request may close this issue.

6 participants