source-integration icon indicating copy to clipboard operation
source-integration copied to clipboard

Import Latest Data fails with no error

Open kabadi opened this issue 7 years ago • 18 comments
trafficstars

Using the latest 2.1 version (because I'm on IIS 10) when I try and Import Latest Data, the page returned just says 'Import Results'

There are no import results or errors. Note that this is true whether I specify the URL and WebSVN URL correctly or not.

I.e. Even if I set the URL to https:\madeup, it still just says 'Import Results' with no error.

kabadi avatar Jan 08 '18 13:01 kabadi

I've tried to follow this through the code. When it gets to the following code, the $t_svn_cmd is good (I can copy it and paste in a command window and it runs ok) but the content of the $t_pipes array is empty

$t_svn_proc = proc_open( $t_svn_cmd, array( array( 'pipe', 'r' ), array( 'pipe', 'w' ), array( 'pipe', 'a' ) ), $t_pipes );

kabadi avatar Jan 09 '18 10:01 kabadi

Is it correct that the second element of the third pipe is 'a'?

Should it be 'w'?

kabadi avatar Jan 09 '18 11:01 kabadi

It actually used to be w, but was changed recently to a via pull request #254.

I don't use SVN or Windows myself, so I could not test this change before merging it, but looking at proc_open documentation, I see now that the descriptorspec definition only allows r and w :

An array describing the pipe to pass to the process. The first element is the descriptor type and the second element is an option for the given type. Valid types are pipe (the second element is either r to pass the read end of the pipe to the process, or w to pass the write end)

Maybe 0c060a63850f1a97edd5ca63ddf99d29936f24a9 should be reverted. @FSD-Christian-ISS care to comment ?

dregad avatar Jan 09 '18 13:01 dregad

I've submitted pull request #261 to change it to w

kabadi avatar Jan 10 '18 14:01 kabadi

Using "a" looks broken on PHP 7.2 running on Windows. Quick test script:

<?php

$t_svn_proc = proc_open(
  			"ping localhost 1>&2",
 			array( array( 'pipe', 'r' ), array( 'pipe', 'w' ), array( 'pipe', 'w' ) ),
  			$t_pipes
);

$t_stderr = stream_get_contents( $t_pipes[2] );
fclose( $t_pipes[2] );
$t_svn_out = stream_get_contents( $t_pipes[1] );
fclose( $t_pipes[1] );
fclose( $t_pipes[0] );
proc_close( $t_svn_proc );

print("Err:");
print($t_stderr);
print("Out:");
print($t_svn_out);

?>

Running this yields:

Err:Out:

Changing the 'a' to an 'w' & re-running provides the expected output.

bright-tools avatar Jan 10 '18 23:01 bright-tools

@bright-tools Thanks for your continued research on this.

Using "a" looks broken on PHP 7.2 running on Windows.

Which is not surprising based on what the docs define, as mentioned in my earlier note https://github.com/mantisbt-plugins/source-integration/issues/259#issuecomment-356285888

Are you implying that a works for earlier PHP versions ?

Anyway based on this discussion and the follow-up in @kabadi's pull request #261, I think the sensible thing to do is simply to revert 0c060a63850f1a97edd5ca63ddf99d29936f24a9.

dregad avatar Jan 11 '18 08:01 dregad

@dregad As you say, not surprising based on the docs. I was just doing a quick experiment to see what the actual behaviour was (seems to be that the stream contents is lost). I've not had chance to test it on any versions other than that which I mentioned.

bright-tools avatar Jan 11 '18 12:01 bright-tools

I've not had chance to test it on any versions other than that which I mentioned.

I ran a quick test over lunch break using WAMP server, I get the same results with 5.6 and 7.0.

dregad avatar Jan 11 '18 13:01 dregad

I'm not into this in detail myself, but a co-worker reported that he added something like this to get this issue fixed:

In file SourceSVN.php, line 337 add if statement like this:

if (stream_get_meta_data($t_pipes[2])["unread_bytes"] > 0)
$t_stderr = stream_get_contents( $t_pipes[2] );

Maybe it helps...

CWBudde avatar Feb 08 '18 15:02 CWBudde

Thanks for the suggestion; it was one of the options which I looked at, but the PHP manual says of unread_bytes,

Note: You shouldn't use this value in a script.

which was enough to scare me off.

bright-tools avatar Feb 08 '18 16:02 bright-tools

Regarding the 'a' vs. 'w' discussion and pull request #254 : we are running Mantis 2.10.0 on Windows (PHP 5.6.17 and Source Subversion Integration plugin 2.1.0). The script (SourceSVN.php) used "w" for the third pipe and we too encountered every now and then hangs (until timeout kicks in after a couple of minutes). This behavior is reproducible when the same SVN revision is used. Seems to be related to the size of the commit. Changing from "w" to "a" seems to solved the issue (I tested it with a SVN revision that would hang before. Now it works.)

BrightLight avatar Feb 28 '18 12:02 BrightLight

I'm having problem with import script hang. Sometimes very small repo can be imported (10 changesets) but often it hangs. Problem started when i updated two years instalation to current (I updated linux, mantis and all plugins in one go, figures)

Fixed with #254

rattkin avatar Apr 27 '18 07:04 rattkin

The change from "w" to "a" on the third array fixed the issue for me too. The script was loading without log a long time. After changing the character the import was possbile and successful for many repositories.

sebastianbrosch avatar Sep 21 '18 20:09 sebastianbrosch

Ubuntu 17, mantis 2.20, - had to change the 'w' to an 'a' as well, otherwise it just hangs

spmeesseman avatar Jun 16 '19 03:06 spmeesseman

While we found a fix for ourself, it seems still not solved in the long run. Any ideas how we can solve this once and for all?

CWBudde avatar Sep 02 '19 16:09 CWBudde

This is my attempt to address the issue in a platform independent way. The main idea is to get rid of stream_get_contents completely and replace it

It seemed to work OK from the limited amount of testing that I was able to do, but it was not sufficient to satisfy me that it's completely robust. Unfortunately I'm no longer using Mantis, so addressing this is languishing down on my task list.

bright-tools avatar Sep 02 '19 20:09 bright-tools

Looks promising. Hopefully this (or anyother approach) will be included soon so that any future plugin update won't stop this function from working correctly. It's especially frustrating when working as a software freelancer, when this feature stops working and you need to persuade the sys-admin of the company to patch some lines every now and then after an update.

CWBudde avatar Sep 02 '19 20:09 CWBudde

@bright-tools sorry to hear you stopped using Mantis, and thank you for posting the link with your feature branch. Hopefully one or more of the growing number of people who land here, will take things over and get this code to a mergeable state.

dregad avatar Sep 06 '19 12:09 dregad