Fix for #747 'git fetch' sporadically hangs, GitServiceExecutor now asynchronous.

[ijd02]

Fixed git deadlock bug in GitServiceExecutor. Fault due to stdIn & stdOut buffers in the git.exe filling and blocking further processing. Handling in GitServiceExecutor now asynchronous i.e. Bonobo is continually emptying the buffers even while its sending data to git. Link to initial issue: https://github.com/jakubgarfield/Bonobo-Git-Server/issues/747

[willdean]

Thanks for this. There are a couple of issues I would like to discuss: (Note that I didn't write the original code, so I'm not defensive about it in the slightest!)

• Is it necessary for both the input and the output stream to be async like this? If we start the output read before writing the input data, then the input write could probably stay synchronous, like it used to be?

• If we're going to fix-up this broken routine, we should probably also make sure we're reading stderr at the same time as stdout, because otherwise is there still some kind of possible deadlock in the situation where a lot of stderr output occurs? (See the last sample on https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.standardoutput?view=netframework-4.8 )

• I find myself a little uncomfortable about the mixture of an old-style pre-Task API (Process), modern Task-async and then the Thread.Sleep() and the Task.Wait() call. I wonder if it's better to avoid the CopyAsync and just use the old-style Process async methods, so avoid mixing approaches here? This is not a fully-thought-through position, just gut feel.

But thanks for the PR, anyway, I would definitely like to add this improvement once we're fully happy with it.

[ijd02]

Hi Will,I wondered about the need for input to be async, once fetch was was working I never went back and tested if synchronous would work. Yes fixing stderr would probably wise. I focused on my immediate issue only. I'm not even sure how stderr is being handled currently. I'm totally new to c# and do little Windows programming so the mixture of old and new styles is inadvertent and may be incorrect. While programming and looking up various API documentation I didn't come across any warning about old/new style. What exactly do you mean by "old-style Process async methods"?I did initially try using OutputDataReceived of the Process class but this wasn't viable due to newlines being mangled i.e. not suitable for raw binary data. I didn't totally follow documentation around the async keyword and related functionality, but I'm highly experience with multi-threaded programming in assembler/C/C++ on a range of operating systems. My understanding is that calling process.StandardOutput.BaseStream.CopyToAsync() will create a new thread to execute the operation and terminate once complete. When this thread has read all available bytes from stdout the next attempt to read byte will block the thread until a new byte becomes available. i.e. the operation never terminates except when the process terminates. The only other way I could think of was to manually create thread and write my own loop that continually calls read on stdout.  

This email is not my primary account so responses may be a little delayed, I was also away over the last few days. Regards, Ian.

On Friday, 5 July 2019, 08:51:10 am UTC, Will Dean <notifications@github.com> wrote:  

Thanks for this. There are a couple of issues I would like to discuss: (Note that I didn't write the original code, so I'm not defensive about it in the slightest!)

• Is it necessary for both the input and the output stream to be async like this? If we start the output read before writing the input data, then the input write could probably stay synchronous, like it used to be?

• If we're going to fix-up this broken routine, we should probably also make sure we're reading stderr at the same time as stdout, because otherwise is there still some kind of possible deadlock in the situation where a lot of stderr output occurs? (See the last sample on https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.standardoutput?view=netframework-4.8 )

• I find myself a little uncomfortable about the mixture of an old-style pre-Task API (Process), modern Task-async and then the Thread.Sleep() and the Task.Wait() call. I wonder if it's better to avoid the CopyAsync and just use the old-style Process async methods, so avoid mixing approaches here? This is not a fully-thought-through position, just gut feel.

But thanks for the PR, anyway, I would definitely like to add this improvement once we're fully happy with it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.