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

Import Latest Data fails with no error #259

Open
kabadi opened this issue Jan 8, 2018 · 18 comments
Open

Import Latest Data fails with no error #259

kabadi opened this issue Jan 8, 2018 · 18 comments
Labels

Comments

@kabadi
Copy link

kabadi commented Jan 8, 2018

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
Copy link
Author

kabadi commented Jan 9, 2018

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
Copy link
Author

kabadi commented Jan 9, 2018

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

Should it be 'w'?

@dregad dregad added the SVN label Jan 9, 2018
@dregad
Copy link
Member

dregad commented Jan 9, 2018

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 0c060a6 should be reverted. @FSD-Christian-ISS care to comment ?

@kabadi
Copy link
Author

kabadi commented Jan 10, 2018

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

@bright-tools
Copy link
Contributor

bright-tools commented Jan 10, 2018

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
Copy link
Member

dregad commented Jan 11, 2018

@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 #259 (comment)

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

dregad added a commit that referenced this issue Jan 11, 2018
This reverts commit 0c060a6 (#254).

As per discussion in #259, use of 'a' for pipe type is not documented in
the PHP manual [1], and the proc_open() call does not work with it
(STDERR returns no output).

[1]: http://php.net/manual/en/function.proc-open.php
@bright-tools
Copy link
Contributor

@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
Copy link
Member

dregad commented Jan 11, 2018

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
Copy link

CWBudde commented Feb 8, 2018

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

rattkin commented Apr 27, 2018

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
Copy link

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
Copy link
Member

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

@CWBudde
Copy link

CWBudde commented Sep 2, 2019

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
Copy link
Contributor

bright-tools commented Sep 2, 2019

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
Copy link

CWBudde commented Sep 2, 2019

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
Copy link
Member

dregad commented Sep 6, 2019

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

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

No branches or pull requests

8 participants