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

[superspawn] Convert exceptions thrown by child_process.spawn into rejections #66

Merged
merged 2 commits into from Apr 1, 2019

Conversation

@raphinesse
Copy link
Contributor

commented Apr 1, 2019

Motivation and Context

At least until Node.js 8, child_process.spawn will throw exceptions
instead of emitting error events in certain cases (like EACCES).

Description

To preserve the async nature of superspawn.spawn we have to wrap the execution in try/catch to convert the thrown exceptions into rejections.

Testing

Added regression test for this bug. Does not run on Windows since the only case I could reproduce this was with an EACCES error which usually does not occur in this context there.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change

raphinesse added some commits Apr 1, 2019

Catch leaked exceptions in superspawn and convert them to rejections
At least until Node.js 8, child_process.spawn will throw exceptions
instead of emitting error events in certain cases (like EACCES), Thus we
have to wrap the execution in try/catch to convert them into rejections.

@raphinesse raphinesse requested review from dpogue and erisu Apr 1, 2019

@project-bot project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Apr 1, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Apr 1, 2019

Codecov Report

Merging #66 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
+ Coverage   88.85%   88.87%   +0.02%     
==========================================
  Files          20       20              
  Lines        1803     1807       +4     
  Branches      369      370       +1     
==========================================
+ Hits         1602     1606       +4     
  Misses        201      201
Impacted Files Coverage Δ
src/superspawn.js 88.7% <100%> (+0.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9951b3...9aa6c49. Read the comment docs.

@raphinesse raphinesse changed the title Superspawn catch errors [superspawn] Convert exceptions thrown child_process.spawn into rejections Apr 1, 2019

@raphinesse raphinesse changed the title [superspawn] Convert exceptions thrown child_process.spawn into rejections [superspawn] Convert exceptions thrown by child_process.spawn into rejections Apr 1, 2019

Apache Cordova: Tooling Pull Requests automation moved this from 🐣 New PR / Untriaged to ✅ Approved, waiting for Merge Apr 1, 2019

@dpogue

dpogue approved these changes Apr 1, 2019

@raphinesse raphinesse merged commit 469e7b4 into apache:master Apr 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Apr 1, 2019

@raphinesse raphinesse deleted the raphinesse:superspawn-catch-errors branch Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.