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

Manually closing streams #222

Merged
merged 2 commits into from
Aug 11, 2015
Merged

Manually closing streams #222

merged 2 commits into from
Aug 11, 2015

Conversation

JulianLaval
Copy link
Contributor

Solves the stream closing issue introduced by pull #214.

According to the Node Docs, process.stderr and process.stdout are closed when the process exits - we can listen to this and close the stdoutStream accordingly

@summer4096
Copy link
Contributor

I fear this could reintroduce the bug that #214 fixed.

Perhaps try something like this:

var stdoutEnded = false, stderrEnded = false;
function tryClosing() {
  if (stdoutEnded && stderrEnded) {
    stdoutStream.end();
  }
}
childProcess.stdout.on('end', function() {
  stdoutEnded = true;
  tryClosing();
});
childProcess.stderr.on('end', function() {
  stderrEnded = true;
  tryClosing();
});

@JulianLaval
Copy link
Contributor Author

@devtristan, whilst this did not explicitly reintroduce the bug fixed by #214, it was indeed misguided in its reliance on process.stdout.

Your solution is much more elegant and relevant; applying your fix + outputting to console on stdStream.end() seems to confirm the validity of this fix in the context of chained git commands.

Pull request updated accordingly!

@arturadib
Copy link
Collaborator

thanks @JulianLaval and @devtristan !

arturadib added a commit that referenced this pull request Aug 11, 2015
@arturadib arturadib merged commit 3bbe153 into shelljs:master Aug 11, 2015
@arturadib
Copy link
Collaborator

this is now published in v0.5.3

@JulianLaval JulianLaval deleted the patch-1 branch August 11, 2015 20:56
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.

3 participants