Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

fix: catch worker-farm errors #372

Closed

Conversation

the-ress
Copy link

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update (if you have an idea how to test this without hacks or major changes, let me know)
  • typo fix
  • metadata update

Motivation / Use-Case

When the worker process fails to spawn, (and only when using cache), the error turns into an unhandled promise rejection and uglifyjs-webpack-plugin hangs and never finishes.

This PR fixes that by catching the error and handling it the same way as in the non-concurrent path.

Unhandled rejection Error: spawn ENOMEM
    at ChildProcess.spawn (internal/child_process.js:313:11)
    at exports.spawn (child_process.js:503:9)
    at Object.exports.fork (child_process.js:104:10)
    at fork (node_modules/worker-farm/lib/fork.js:17:36)
    at Farm.startChild (node_modules/worker-farm/lib/farm.js:106:16)
    at Farm.processQueue (node_modules/worker-farm/lib/farm.js:279:10)
    at Farm.addCall (node_modules/worker-farm/lib/farm.js:307:8)
    at Farm.<anonymous> (node_modules/worker-farm/lib/farm.js:38:10)
    at _class.boundWorkers (node_modules/uglifyjs-webpack-plugin/dist/uglify/index.js:71:24)
    at enqueue (node_modules/uglifyjs-webpack-plugin/dist/uglify/index.js:96:17)

Breaking Changes

I don't think there are any.

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Oct 26, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for PR, can you add tests and comment in code about problem (minimum)?

@the-ress
Copy link
Author

How would you approach testing this?

@alexander-akait
Copy link
Member

@the-ress you need mock workerFarm (example how we mock cachache https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/master/test/cache-option.test.js#L73), throw error into mock, set parallel: true and snapshot error (https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/master/test/cache-option.test.js#L80). It is allow to ensure what our code works as expected and allow testing this code on CI to avoid problem for other developers.

@codecov
Copy link

codecov bot commented Nov 5, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
+ Coverage   97.69%   97.71%   +0.02%     
==========================================
  Files           5        5              
  Lines         260      263       +3     
  Branches      102      102              
==========================================
+ Hits          254      257       +3     
  Misses          6        6
Impacted Files Coverage Δ
src/TaskRunner.js 97.87% <100%> (+0.14%) ⬆️

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 bf36e21...84316f4. Read the comment docs.

@the-ress
Copy link
Author

the-ress commented Nov 5, 2018

@evilebottnawi I added the tests and comment.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good, need move test in https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/master/test/parallel-option.test.js (group tests together) and update snapshot for windows (use -u flag)

@the-ress
Copy link
Author

the-ress commented Nov 6, 2018

@evilebottnawi Does the Windows build pass on master?

@alexander-akait
Copy link
Member

@the-ress yes, sometimes appveyor have problem with caching npm/yarn on their side, need just update snapshot

@alexander-akait
Copy link
Member

/cc @the-ress

@alexander-akait
Copy link
Member

Close in favor #380, thanks for PR and sorry for delay (many works)

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

Successfully merging this pull request may close these issues.

None yet

3 participants