Skip to content

Manually closing streams#222

Merged
arturadib merged 2 commits intoshelljs:masterfrom
JulianLaval:patch-1
Aug 11, 2015
Merged

Manually closing streams#222
arturadib merged 2 commits intoshelljs:masterfrom
JulianLaval:patch-1

Conversation

@JulianLaval
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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