Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

fail - always read stdout and stderr from forked commands #13

Closed
johnstevenson opened this issue Jan 22, 2014 · 44 comments
Closed

fail - always read stdout and stderr from forked commands #13

johnstevenson opened this issue Jan 22, 2014 · 44 comments

Comments

@johnstevenson
Copy link
Member

This PR #11 fails and hangs Far Manager when a Git menu command is clicked. Tested on Vista (x86).

So the problem must be with the pipes/redirected output since it works pre PR, albeit reporting spurious error messages. Maybe it is waiting on a duplicated handle that should have been closed? Perhaps it combines stdout and stderr.

Incidentally, this will not actually compile in Visual Studio because the *debug variable is not declared at the beginning of the exec_program_v function which MSVC requires for .c files.

@dscho
Copy link
Member

dscho commented Jan 22, 2014

So the problem must be with the pipes/redirected output since it works pre PR, albeit reporting spurious error messages. Maybe it is waiting on a duplicated handle that should have been closed? Perhaps it combines stdout and stderr.

It would be real helpful if you could investigate further, i.e. compile Git-Cheetah (using the msysGit net installer) with debug messages that show where it hangs.

Incidentally, this will not actually compile in Visual Studio because the *debug variable is not declared at the beginning of the exec_program_v function which MSVC requires for .c files.

Patches welcome.

@johnstevenson
Copy link
Member Author

It would be real helpful if you could investigate further

Of course. I just need to get the next release of ComposerSetup out and am waiting on a response on the msysGit mailing list to finish off: https://groups.google.com/forum/#!topic/msysgit/P7a2MVF0ok8

Patches welcome.

No problem, but I couldn't see the point of patching something that doesn't work at this stage.

@dscho
Copy link
Member

dscho commented Jan 22, 2014

Thanks for the bump, I responded to that mail.

BTW I did not realize that you are a contributor because I completely forgot to add you to the GitHub team! Sorry! This is fixed now.

@dscho
Copy link
Member

dscho commented Mar 21, 2014

@johnstevenson is this addressed by #14?

@johnstevenson
Copy link
Member Author

No, it still hangs. Sorry, I haven't had a chance to look at this yet.

@dscho
Copy link
Member

dscho commented Mar 22, 2014

@johnstevenson even after pulling master? That would be bad.

@johnstevenson
Copy link
Member Author

Sadly, yes. Tested with master at da97444

@dscho
Copy link
Member

dscho commented Mar 22, 2014

Bummer. I actually got a chance to test this myself and I can reproduce. Darn. I'm out of Git time budget this week (thanks to some non-fun administrativa yesterday).

@dscho
Copy link
Member

dscho commented Mar 23, 2014

Could you please give #15 a try?

@johnstevenson
Copy link
Member Author

Sorry, but it still hangs - I haven't had the chance to debug it yet.

More worryingly, it crashes Explorer/Far Manager when you click on the Git Bash entry. Note that I have built the dll with Visual Studio and it crashes calling dup2.

@dscho
Copy link
Member

dscho commented Mar 23, 2014

Whoa. It does not do that here. (I built with gcc.) Do you have any idea what parameters dup2() gets?

To be precise, for me it only fixed things in Explorer if I killed the Explorer and restarted it. The usual make uninstall uninstall-user && make install-user did not fix it.

@mwpowellhtx
Copy link

To be clear, how does "killing Explorer and restarting it" fix anything? If Explorer is hanging, blocking on some call or another expecting some I/O or something, I believe that's the whole point of the issue. Those of us who do use the Explorer context menu find that unacceptable. For me, it's a pain, but there's a workaround. Would be nice if it worked through context menu as well, but can be dealt with other ways. Thank you...

@dscho
Copy link
Member

dscho commented Mar 23, 2014

To be clear, how does "killing Explorer and restarting it" fix anything?

It fixes Explorer's failure to pick up an updated Cheetah.

@mwpowellhtx
Copy link

Okay. Well, I am not a Git Cheetah much less shell extension Subject Matter Expert. But something tells me as an extension, Git Cheetah is to blame, not Explorer.

@dscho
Copy link
Member

dscho commented Mar 23, 2014

@mwpowellhtx I take it you did not build the code from this pull request.

@mwpowellhtx
Copy link

I'm just reporting the bug. It sounds like it's already been fixed. You should discuss that issue with the authors of the fix.

@dscho
Copy link
Member

dscho commented Mar 23, 2014

@mwpowellhtx you claimed that you wanted to be an official contributor. Contributors contribute code, test fixes and help others with their issues.

BTW I am in contact with the author of the fix.

The author is yours truly.

@mwpowellhtx
Copy link

I was under the impression there was not already a fix available. What I proposed was counting the cost, not committing whether I would. But since there already is fix, I expect you are testing your own code. That is all.

@johnstevenson
Copy link
Member Author

Calling dup2(-1, 1) is the problem: https://github.com/msysgit/Git-Cheetah/blob/far-manager/common/exec.c#L69

The function has been passed the DETACHMODE flag, so devnull never gets changed from an invalid handle value:

if ((!output || !error_output) && !(DETACHMODE & flags))
      devnull = open("/dev/null", O_WRONLY);

@dscho
Copy link
Member

dscho commented Mar 24, 2014

@mwpowellhtx will you be contributing something else than words, too? Of course you must test the fix on your side, otherwise you risk that your problem is not fixed. It is a bit sad that this does not go without saying.

@dscho
Copy link
Member

dscho commented Mar 24, 2014

@johnstevenson whoops. you're right, devnull is set to -1 by default, not 0, therefore my beautiful if (devnull) did not catch it.

I just force-pushed a new version; could you please give this one a try, too?

@johnstevenson
Copy link
Member Author

Okay, we are not crashing Explorer now by calling Git Bash. We are still causing Far Manager to hang forever, if you try and switch branches for example.

This all seems extraordinarily complicated - if we want to invoke Git Bash from the context-menu why can't we use a simple ShellExecute?

@dscho
Copy link
Member

dscho commented Mar 24, 2014

@johnstevenson in the short run, it would help fix things, I agree, but in the long run we have an incredible technical debt: Git-Cheetah is supposed to be cross-platform.

I will have a look at the switching branches problem next week if nobody beats me to it.

@dscho
Copy link
Member

dscho commented Mar 25, 2014

@johnstevenson I invested half of today's Git time budget to try to reproduce the failure you described, but I failed! FarManager does not hang here if I switch branches. It does exactly what I want it to do, and it is actually very fast doing it.

To recapitulate what I did:

  1. quit Far Manager (otherwise the final linking step will fail because Far Manager uses the .dll, still)
  2. rebuild Git-Cheetah from scratch, on the far-manager branch: make uninstall uninstall-user clean install-user

Since I have a 64-bit VM and a 32-bit Far Manager running inside, I did not pass the W64=1 setting (otherwise I would generate a 64-bit shell extension that the 32-bit Far Manager does not use).

To make sure everything is still groovy here, I also ran make W64=1 uninstall uninstall-user clean, killed the explorer via the Task Manager, then ran make W64=1 install-user and restarted the explorer (in the Task Manager, on the Applications tab, via the New Task... button).

If this still does not work for you, I really need to ask you to look deeper into the issue because I have no way to reproduce the problem here anymore (at least not after my most recent update to the far-manager branch yesterday).

@johnstevenson
Copy link
Member Author

I am in the process of setting up a new dev box, so could you zip and email
me the 32 and 64-bit dlls (john-stevenson at blueyonder.co.uk) and I'll
test those, in case this is a VS compilation issue.

On 25 March 2014 13:19, dscho notifications@github.com wrote:

@johnstevenson https://github.com/johnstevenson I invested half of
today's Git time budget to try to reproduce the failure you described, but
I failed! FarManager does not hang here if I switch branches. It does
exactly what I want it to do, and it is actually very fast doing it.

To recapitulate what I did:

  1. quit Far Manager (otherwise the final linking step will fail
    because Far Manager uses the .dll, still)
  2. rebuild Git-Cheetah from scratch, on the far-manager branch: make
    uninstall uninstall-user clean install-user

Since I have a 64-bit VM and a 32-bit Far Manager running inside, I did
not pass the W64=1 setting (otherwise I would generate a 64-bit shell
extension that the 32-bit Far Manager does not use).

To make sure everything is still groovy here, I also ran make W64=1
uninstall uninstall-user clean, killed the explorer via the Task Manager,
then ran make W64=1 install-user and restarted the explorer (in the Task
Manager, on the Applications tab, via the New Task... button).

If this still does not work for you, I really need to ask you to look
deeper into the issue because I have no way to reproduce the problem here
anymore (at least not after my most recent update to the far-managerbranch yesterday).

Reply to this email directly or view it on GitHubhttps://github.com//issues/13#issuecomment-38562676
.

@dscho
Copy link
Member

dscho commented Mar 25, 2014

@johnstevenson
Copy link
Member Author

Thanks. You will be pleased to know that this was a vs compile issue. Your
32 and 64-bit dlls worked fine in Vista 32 and Win 8.1 64 (in both
environments). That is they did not hang their appropriate Far Manager.

Strangely, my vs 32 and 64-bit dlls also work fine on Win 8.1 64 (in both
environments), but the 32-bit causes Far Manager to hang on Vista 32.

Sorry to have used up your time on this (though I have spent quite a bit
myself). Incidentally, I am baffled that you were testing a 32-bit Far
Manager on Win 64 when this is not a possible configuration.

@dscho
Copy link
Member

dscho commented Mar 26, 2014

Thank you so much @johnstevenson! I do appreciate all the help you provided, and I would like to ask for a little more: do you think you can dig deeper into the MSVC problem? I am really, really curious what goes wrong there (even if I am unlikely to compile Cheetah with anything but gcc anytime soon).

@johnstevenson
Copy link
Member Author

Don't worry, I do not need appreciation! I just couldn't resist a little banter regarding your, understandably, precious git time. I am part way through an output-redirection implementation in a Win32 stylee, just to prove to myself that it is/is not a case of Far manager waiting for an unclosed/unreadable pipe. So we will see what that will bring. Otherwise I am at a bit of a loss as to why this happens.

@johnstevenson
Copy link
Member Author

As I had half expected it is an output redirection issue (possibly due to a bad CRT implementation). Using Win32 handles rather than C file-descriptors cures the problem with an MSVC build - ie Far Manager does not hang on Vista 32.

https://github.com/johnstevenson/Git-Cheetah/tree/winexec

I can only assume that the reason the normal (ie using C file-descriptors) MSVC builds work on Windows 8.1 is that the OS must intervene in some way and tidy up the handles. Of course, this is only a wild guess.

I have also tested building on VS2012 and VS2013 in case the CRT had changed, but to no avail. What a shame, because using VS is a quick way to get up and running to test and debug the extension. At least, that is how I got involved.

@dscho
Copy link
Member

dscho commented Mar 28, 2014

@johnstevenson this is really good stuff! And I think we can get this into a real nice shape and include it in Git-Cheetah.

As you know, I am interested in maintaining the cross-platform nature of Git-Cheetah, but that is easy! All we need to do is modify your code slightly to offer a drop-in replacement for the function exec_program_v declared in exec.h. For that, I actually do not think that we need a winexec.h at all (because the definitions are actually only used inside winexec.c; we can define the constants and functions in the appropriate order in winexec.c and make them static), and I'd also like to move winexec.c to common/, with the usual #ifdef _WIN32 guards. We also do not really need a new name for the function, but simply put the current definition of exec_program() between ifndef _WIN32 guards and rename the win_exec() function to exec_program_v().

What do you think?

@dscho
Copy link
Member

dscho commented Mar 28, 2014

Just in case you're okay with that plan, I started working on it: master...winexec (but got interrupted; will continue later).

@johnstevenson
Copy link
Member Author

Yes I am fine with it - thanks for setting up and re-formatting. Have you got any code guidelines - tabs or spaces, function names etc?

How is this going to work? I want to make a slight improvement to the code, so I guess a PR is the best way?

@johnstevenson
Copy link
Member Author

Forget the above (a plan to use SetStdHandles didn't work). Incidentally it will not compile on VS 2010 without a #pragma once top line in winexec.c, due to this bug:
http://connect.microsoft.com/VisualStudio/feedback/details/651405/pch-warning-after-installing-sp1

This does not happen on VS2012.

@dscho
Copy link
Member

dscho commented Mar 28, 2014

Good point you remind me of: I wanted to ask whether you can make sure it compiles on VS, and works, still...

Regarding the #pragma once statement, how does

#pragma once
look to you?

If you're okay with it, I would push this branch (with just one addition I just made to make it link with gcc) to master and take it from there, PRs welcome everytime you have something!

@dscho
Copy link
Member

dscho commented Mar 28, 2014

Oh, as to coding style, I tried to follow https://github.com/msysgit/git/blob/master/Documentation/CodingGuidelines, but not too strictly. I do care about tabs instead of spaces because having mixed indentation makes rebasing and merging a major PITA. Likewise, I think it makes sense to keep with the convention of putting the opening curly brackets on the end of the same line as the if statement. But I'm not a stickler to stripping curly brackets from single-line code blocks, for example, because all I really care about is making working with the code easy. Well, and it helps if it looks nice. ;-)

@johnstevenson
Copy link
Member Author

It looks lovely! Actually it did subsequently build without the #pragma line (huh!) but it would be good to keep to avoid the warning (and make the Intelli bit work).

Compiles on VS 2010 and works with Far Manager on Vista 32. Haven't tested 64-bit. Incidentally, you can drop the #include <stdint.h> line - it was a leftover from something or other.

I changed the code in path_lookup_prog a little from the orginal lookup_prog function in mingw.c. You might want to check that I have got this right.

@dscho
Copy link
Member

dscho commented Mar 28, 2014

Merged! Your path_lookup_prog looked okay, I would probably have done it with strbufs now... But it works. Probably ;-)

@dscho
Copy link
Member

dscho commented Mar 28, 2014

Oh yeah, I should close this ticket, too ;-)

@dscho dscho closed this as completed Mar 28, 2014
@johnstevenson
Copy link
Member Author

Eeek! I am not freeing orig, created here:

orig = xstrdup(envpath);

Thanks for the code guidelines. I did look there but was once told off by one of the msysgit guys for using spaces in a file that had 80% spaces and 20% tabs! So I was not sure.

You can happily remove my braces from single-line code-blocks if you wish.

@dscho
Copy link
Member

dscho commented Mar 28, 2014

I am not freeing orig [...]

Yeah, for a long-running shell extension, this can cause problems. Would you care for fixing this in a pull request, please? I ran out of Git time today...

I [...] was once told off by one of the msysgit guys for using spaces

I hope it was not me. If it was, I apologize and hope that you will be happy to learn that I am much nicer now.

You can happily remove my braces from single-line code-blocks if you wish.

As I said, I am not a stickler for that because it does not cause merge conflicts. Most of the time ;-)

@johnstevenson
Copy link
Member Author

No it wasn't you (and it was only a minor, if somewhat baffling, reprimand). You have always been very pleasant - although you do have an annoying knack of getting me to do stuff !! I have actually learned a lot, so thanks.

@kusma
Copy link
Member

kusma commented Mar 29, 2014

Awesome work, thanks!

@dscho
Copy link
Member

dscho commented Mar 29, 2014

@johnstevenson if that is the only thing you find annoying with me, I can live with that :-)

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

No branches or pull requests

4 participants