#336: fix hanging git subprocess in case of too much output on stdout/stderr. #396
Conversation
|
i see that the conflict relates to a new timeout-setting for the native-git-provider. |
|
I fixed the conflicts locally, there came up two more questions when doing that:
Nevertheless i would recommend to merge the changes. |
|
Thanks for creating a PR for that (greatly appreciated)! |
|
Hi, The BigDiff Tests passes, but that passed manually on my side before any changes. From my perspective the only thing I currently would ask for is to condense down the current MR to either the version I created (hopefully i didn't miss anything), or achieve the same in any other shape or form. Regardless on how you want to proceed I think it should also be verified that the bug is gone (that's the entire goal here). Besides that the changes seem reasonable and thus i would be to happy to merge the condensed form (where one can see in the diff what changed). |
|
Indeed the methods have been rearranged by my IDE automatically, this is a setting i am using for other projects, which is somehow not suitable for multi-peer-projects. I am sorry for that. About the BigDiffTest: its also not reliable reproducing the issue on my machine. In 1 of 4 cases it passes with the old implementation. Not sure why, i assume that the JDK dynamically allocates pipe-sizes. I try to look into your condensed patch later on this week. BR |
|
The auto-formatting changes happen for me all the time too....so nothing to be sorry about it happens :-) Interesting, let me know how you would like to proceed and in particular if the final patch (whatever we conclude is the right thing) works as expected. |
…shed. In case the buffer is full, the subprocess waits infinitly. Therefore the prototype implementation flushes the subprocess buffer by reading from stdout/stderr on background threads.
andreas-eternach
force-pushed
the
master
branch
from
ad203de to
5841708
Compare
|
Hello again, i fixed the test case, you could probably not repdroduce because it used jgit instead of native git. This change probably happened when i merged HEAD into the PR. I now reverted the PR via a force-push, actually using your cleaned patch, only additional with the BigDiffTest, which i enabled and which should pass now. |
| outGobbleThread.start(); | ||
| errorGobbleThread.start(); | ||
| if (!proc.waitFor(nativeGitTimeoutInMs, TimeUnit.MILLISECONDS)) { | ||
| proc.destroy(); |
There was a problem hiding this comment.
Nice catch!
I was somehow under the impression that proc.waitFor throws an exception when the timeout has been reached, but that doesn't seem to be the case (interesting)...
I'm wondering if we also would need to terminate the threads in weird conditions...I would assume once the process is killed the input / output stream would be empty and then the threads would automatically terminate...mhhh
…lify the entire process launching
Context
#336: fix hanging git subprocess in case of too much output on stdout/stderr.
The previous implementation read the output after the subprocess finished. In case the buffer is full, the subprocess waits infinitly.
Therefore the new implementation flushes the subprocess buffer by reading from stdout/stderr on background threads.
Contributor Checklist
mvn clean packagecheckstylecoding style definition:mvn clean verify -Pcheckstyle -Dmaven.test.skip=true -B