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

Git: run detached #1314

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Git: run detached #1314

wants to merge 1 commit into from

Conversation

wmertens
Copy link
Contributor

This prevents git hanging for asking ssh passphrases, like when you run it inside vscode with an empty ssh-agent.

Now when I run SSH_AUTH_SOCK= npm start, it immediately errors in the web UI instead of hanging.

@ylecuyer
Copy link
Contributor

Ahah nice I often have this error ;)
Though I think the detached: false was here on purpose ? I hope there is a test for it so we can see what this change breaks :p

@campersau
Copy link
Collaborator

It was added in #1192 but I am not sure what issues @jung-kim had.

fix issue with detached git processes on some OS and timeout not being enforced.

@campersau
Copy link
Collaborator

I am not sure if detached: true is required when we also always close the stdin.

I have looked at vscode (they also use git via spawn) and they use stdio = ['ignore', null, null] to prevent redirecting stdin which I like a bit better.

@wmertens
Copy link
Contributor Author

I only added the detached: true when closing stdin wasn't sufficient. In VS Code I have the same problem, actually. I should open an issue for that 😅

@jung-kim
Copy link
Collaborator

Hmm, I understand the issue but I don't think detached: true means that that hang process will continued to be hanged even after parents process has exited.

I don't think that is a good idea as there maybe just hung threads running without user's awareness, especially even after ungit process is dead.

There was a PR or code changes I've done long time ago around doing timeouts on git processes but I think I've lost it by now. I think something similar might be a better solution

@wmertens
Copy link
Contributor Author

Ok, so if it runs detached there's a chance it hangs for some reason other than waiting for a password and then it won't get killed? Doesn't the timeout work for that? (Didn't check)

@jung-kim
Copy link
Collaborator

Yes it can cause orphaned threads.

I could depends on the implementations.

This prevents git hanging for asking ssh passphrases
@wmertens
Copy link
Contributor Author

wmertens commented Feb 1, 2021

In the docs it says:

By default, the parent will wait for the detached child to exit. To prevent the parent from waiting for a given subprocess to exit, use the subprocess.unref() method. Doing so will cause the parent's event loop to not include the child in its reference count, allowing the parent to exit independently of the child, unless there is an established IPC channel between the child and the parent.

So I think running detached is safe.

Closing stdin is not enough, it will still prompt me for keys when I run SSH_AUTH_SOCK= npm run start

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

Successfully merging this pull request may close these issues.

4 participants