mantisbt-plugins / source-integration

Source control integration plugin framework for MantisBT, including support for Github, Gitlab, Bitbucket, Gitea, Gitweb, Cgit, Subversion, Mercurial and more
http://noswap.com/projects/source-integration/
MIT License
181 stars 130 forks source link

Import Latest Data fails with no error #259

Open kabadi opened 6 years ago

kabadi commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

Should it be 'w'?

dregad commented 6 years ago

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 ?

kabadi commented 6 years ago

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

bright-tools commented 6 years ago

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.

dregad commented 6 years ago

@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.

bright-tools commented 6 years ago

@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.

dregad commented 6 years ago

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.

CWBudde commented 6 years ago

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...

bright-tools commented 6 years ago

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.

BrightLight commented 6 years ago

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.)

rattkin commented 6 years ago

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

sebastianbrosch commented 5 years ago

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.

spmeesseman commented 5 years ago

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

CWBudde commented 5 years ago

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?

bright-tools commented 5 years ago

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.

CWBudde commented 5 years ago

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.

dregad commented 5 years ago

@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.