Updates to proc_open pipe handling

[kabadi]

Changed the descriptorspec for the stderr pipe to 'w' (it was 'a' but this seems only applicable when streaming to a file, not a pipe)
Reversed the order in which the stderr and stdout pipes are read to get around an apparent bug in the proc_open

[kabadi]

Fixes issue #241 and #259

[bright-tools]

I'm wondering if the more robust approach is to use stream_set_blocking($t_pipes[2], 0). According to various other reports of problems with proc_open, reversing the calls may work for some situations, but not all (i.e. ones where stderr has content, but stdout does not).

[dregad]

@bright-tools regarding your suggestion, according to user notes in PHP manual stream_set_blocking() does not work under windows for pipes opened by proc_open().

Maybe an alternative solution would be to use a file to read STDERR ?

[dregad]

Commit 0c060a6 that introduced use of a in STDERR pipe descriptor has been reverted in 1e8407f.

I'll leave this PR open for now, to continue the discussion on fixing the blocked stream.

[bright-tools]

@dregad, I subsequently saw that, too. I've looked into an alternative approach (mentioned in comments on #241) and it seems to work in a limited test, but not yet confident enough to suggest it as being the right way to go forward.

[agolks]

there should be a solution for this issue.
ok, i'm no php crack, but it took me 1 day now to understand, why importing changesets just hang.
a single stream_set_blocking($t_pipes[2], 0) solved this issue.

for me, the unblock should just be executed.
either, it works, and all are happy. (like me).
or it does not work, and you're not unhappier as before, as it still is not working.

so it only gets better.
tldr;
you really should accept the unblock of stderr!

[obmsch]

I ran into this problem the first time yesterday. I am running a post-commit hook (POST to checkin.php)
and this was an unusual large commit (xml-size ~140KB). The script deadlocked until the servers timeout
kicked in (~15min) and killed it (Win10/1709-x64, IIS10, PHP7.1/x64, SourceIntegration 2.1.0, SVN 1.9.7).

Assuming STDERR will never exceed BUFFERSIZE (unlikely but possible), reversing the order the pipes are
read in SourceSVN.php should work and does for me. As this is a workaround and no "real solution", an
appropriate comment should be applied.

		# Get output of the process & clean up
		# Due to limitations (at least on windows) for the buffering of
		# pipes and possible resulting deadlocks the order the pipes are
		# read is crucial. The reordering (STDOUT before STDERR) will not(!)
		# prevent a deadlock, if the output of the subprocess to STDERR
		# exceeds the max buffered size on STDERR.
		$t_svn_out = stream_get_contents( $t_pipes[1] );
		fclose( $t_pipes[1] );
		$t_stderr = stream_get_contents( $t_pipes[2] );
		fclose( $t_pipes[2] );

[bright-tools]

I finally got round to cleaning up & testing what I hope is a robust way to deal with this. I wrote an off-line "stress test" for the code which seems to show it dealing with a large quantity of data spread across the two streams.

Only tested in Windows at the moment, needs more testing before submitting a pull request.

[fhaut]

Today, using the mantisbt-plugins:master the import of +- 120 revisions causes a php page hangs on my production environment. After searching and applying the pull request #254 solves the problem.

Change from:
array( array( 'pipe', 'r' ), array( 'pipe', 'w' ), array( 'pipe', 'w' ) ),
TO:
array( array( 'pipe', 'r' ), array( 'pipe', 'w' ), array( 'pipe', 'a' ) ),

uname -a
Linux webserver 4.4.0-119-generic #143-Ubuntu SMP Mon Apr 2 16:08:24 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

php --version
PHP 7.0.28-0ubuntu0.16.04.1 (cli) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2017 Zend Technologies
with Zend OPcache v7.0.28-0ubuntu0.16.04.1, Copyright (c) 1999-2017, by Zend Technologies

[dregad]

@fhaut I think it's been established by now that using a for the stderr pipe is not valid (see 1e8407f and discussion in #259)

@bright-tools where do you stand with those tests ? FYI @raspopov submitted another PR for this issue (see #268) but that relies on stream_set_blocking() so based on previous discussion I don't think it's the right solution.

[raspopov]

I noticed that closing STDIN and reading STDOUT before STDERR helps a lot in all cases including STDERR-only and STDOUT-only results of SVN run.

BTW "a" is not working.

[dregad]

@bright-tools were you able to make any progress on this issue ?

[dregad]

@bright-tools ping

[DavidHopkinsFbr]

Can confirm this is still an issue on Windows. I followed obmsch and raspopov's instructions, and that allowed me to get up and running.

While awaiting a more robust solution, I think it would be a good idea to at least implement the stream order swap in the master branch. The plugin couldn't import merges for me at all until I did that. After swapping the order the situation has improved from "broken" to "fragile" - it definitely works when SVN doesn't return errors, and it looks like it should continue to work so long as SVN doesn't return very long error content? I'm a little worried about reverting to the broken state if I deploy a plugin update which doesn't include that change.

Obviously @bright-tools's robust fix would be even better.

[seanm]

We ran into this bug too (I had created #314).

Swapping the two stream_get_contents() "fixed" the issue for us.

Do I understand correctly from reading all the above that there's no robust/correct solution known?

[dregad]

Do I understand correctly from reading all the above that there's no robust/correct solution known?

Yep.

That being said, I would be fine with implementing the swap of streams, since based on the feedback provided here by various users, it would appear to improve the plugin's behavior overall.

Please cast your vote by up/down-voting this post if you agree with this approach.

[seanm]

@dregad so is 7 upvotes enough to swap those statements? :)

[obmsch]

For the standing svn users of us, this should be finally resolved. Sorry but what @bright-tools offers is
beyond my skills on windows, *nix left out anyway.

The simple swap of STDERR and STDOUT is the right way to go! After that a deadlock could only happen, if (svnexe) spills out >MAX_BUFFERED_PIPE_SIZE(STDERR). As unlikely that is, I'd would
rather handle an error then, always better than get stuck on the good.

#333

[seanm]

@dregad so we've got 8 up votes and 0 down votes. Is there anything worse about swapping the streams vs what's in master now? Maybe there's an even better solution somewhere, but that shouldn't stop an incremental improvement, right? Thanks.

[dregad]

Not really, except my lack of available time to actually get to it. Thanks for your patience and understanding.

I have just merged @obmsch's PR #333, and will also close this one as it was basically the same fix, and I don't see any reason to leave it open.

Considering that reordering the order in which the buffers are read is only a workaround / partial fix, the original issue #259 remains open. Maybe one of you SVN guys will find the time to pursue the work done by @bright-tools in #259 (comment) and test his feature branch.