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

Do not read streams more than once #330

Merged
merged 1 commit into from Jun 30, 2019
Merged

Do not read streams more than once #330

merged 1 commit into from Jun 30, 2019

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Jun 26, 2019

Fixes #327
Fixes #321

We read the stdout, stderr and all streams to make them available as a string/buffer to users. We defer it (we wait for execaResult.then() to be called) in case users prefer consuming those streams themselves.

However if execaResult.then() is called twice:

  • the second result will fail, as a stream cannot be consumed twice. I.e. the second call will show the stream as empty.
  • the repeated computation is unnecessary as all execaResult.then() should just follow the same promise.
  • this repeated computation creates issues as consuming streams sets event listeners which are not cleared up because of a bug in pump, which leads to a memory leak and warnings in the console.

Arguably users should not call execaResult.then() several times. However this might happen when execaResult is passed from consumer to consumer, with each consumer doing its own (possibly delayed) logic with it.

This PR fixes this problem by ensuring the streams consumption function is only called once, using the small utility onetime.

This was referenced Jun 26, 2019
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.

result.all is not constant EventEmitter memory leak
2 participants