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

Deadlock when asynchronous error happens on make #3464

Closed
hansl opened this issue Dec 11, 2016 · 4 comments · Fixed by #3479
Closed

Deadlock when asynchronous error happens on make #3464

hansl opened this issue Dec 11, 2016 · 4 comments · Fixed by #3479

Comments

@hansl
Copy link

hansl commented Dec 11, 2016

Reporting a bug

It seems that asynchronously calling the callback with an error during the make step causes a deadlock 100% of the time. This was diagnosed in the webpack-dev-middleware (see webpack/webpack-dev-middleware#156), but it seems that it's happening deeper into webpack, as the issue happens without that plugin.

Also, if the error happens synchronously, the watch process simply exits even if it shows Webpack is watching the files….

Reproduction

Cloning this project (random webpack 2 seed), and applying this patch:

diff --git a/webpack.config.ts b/webpack.config.ts
index 8a0c706..b308d6c 100644
--- a/webpack.config.ts
+++ b/webpack.config.ts
@@ -148,6 +148,16 @@ const commonConfig = function webpackConfig(): WebpackConfig {
     new ForkCheckerPlugin(),
     new DefinePlugin(CONSTANTS),
     new NamedModulesPlugin(),
+
+    new (class ErrorPlugin {
+      apply(compiler) {
+        compiler.plugin('make', function(compilation, cb) {
+          Promise.resolve()
+            .then(() => cb(new Error('THIS IS AN ERROR')));
+        });
+      }
+    })(),
+
     ...MY_CLIENT_PLUGINS
   ];

When doing webpack --watch it will show the error, continue the compilation, then block at 70%, never exit and doesn't rebuild when a file changes.

Changing the error plugin to be synchronous will result in the watcher quitting, which is also not what we want.

@sokra
Copy link
Member

sokra commented Dec 12, 2016

The expected behavior is to show the error and quit, because this is a crititcal error. Throwing an error is not allowed in a async plugin hook, node should crash here.

We can't start watching as we don't know which files to watch. The make hook adds the file dependencies which are watched. So the fileset could be incomplete, and we don't want to start watching a broken fileset.

Notes to me:

https://github.com/webpack/webpack/blob/master/lib/Compiler.js#L97-L98

https://github.com/webpack/webpack/blob/master/bin/webpack.js#L296 add || err here

https://github.com/webpack/webpack/blob/master/bin/webpack.js#L304 remove the condition

Notes to @SpaceK33z

The dev-middleware should error when the Watching fires the failed event. https://github.com/webpack/webpack/blob/master/lib/Compiler.js#L95

The dev-server should quit.

@sokra sokra added this to the 2.1-RC Blocking milestone Dec 12, 2016
@hansl
Copy link
Author

hansl commented Dec 12, 2016

@sokra What would be the recommended way to trigger an error during code generation, but not kill the build. Basically, we know code generation in our case might fail but since we're only adding new code it doesn't matter for the dependency graph of Webpack and we know we can rebuild when a file changes.

@hansl
Copy link
Author

hansl commented Dec 12, 2016

I'm going to try to succeed the make step, keep the error internally) and fail on after-emit or something similar. This should be enough for our use case.

@sokra
Copy link
Member

sokra commented Dec 13, 2016

All errors passed to the callback from Compiler and Compilation hooks are critical errors. You can add errors to the compilation.errors or .warnings array if they are not critical. But you need to make sure that compilation.fileDependencies is set correctly, elsewise watching will not watch the files you used.

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

Successfully merging a pull request may close this issue.

3 participants