Silent freeze if source imports non-existant module

[soryy708]

If browserify is given the watchify plugin, it will silently freeze if there's an import of a non-existent module anywhere in the source files reachable via entries.

I've isolated a minimal complete reproducible example:

src.js:

import foobar from 'someNonExistantPackage';

gulpfile.js:

const gulp = require('gulp');
const watchify = require('watchify');
const browserify = require('browserify');
const vinylSource = require('vinyl-source-stream');
const vinylBuffer = require('vinyl-buffer');

function createBundler() {
    return browserify({
        entries: ['./src.js'],
    }).plugin(watchify);
}

function bundle(bundler) {
    return bundler.bundle()
        .pipe(vinylSource('dst.js'))
        .pipe(vinylBuffer())
        .pipe(gulp.dest('./'))
        .on('error', console.error)
        .on('end', () => { console.log('Done'); });
}

gulp.task('default', () => {
    return bundle(createBundler());
});

What I expect to happen is for an error to be thrown. I should see it either in .on('error', console.error) or by the gulp task crashing. What happens in practice is that the task freezes without printing anything.

If the watchify plugin is removed from the chain, then the task continues and errors successfully:

ParseError: 'import' and 'export' may appear only with 'sourceType: module'

which is expected, because I removed the babel transform from the example to keep it minimal. If babel is added, then dst.js is successfully generated as long as watchify is not used.

This happens to me on:

• browserify version 16.5.0
• watchify version 3.11.1

[soryy708]

I made an automated test bench that reproduces this at various versions of watchify.

The test bench uses the source and gulpfile as said previously. What it tests for specifically is for the process being frozen. The way it works is as follows:

• gulpfile.js is invoked via require('child_process').exec with a timeout option.
• Wait for the process to stop (both in success and in failure, we expect the process to "fail")
• If the process was killed, fail the test. It means that timeout was exceeded, meaning the process froze.

My results are as follows:

• This bug is reproduced in versions 3.3.1 through 3.11.1
• This bug is not reproduced in versions 3.0.0 through 3.3.0

I did not test on versions prior to 3.3.0, but the test bench can be trivially modified to include them in the testing.

[soryy708]

Here's a diff between 3.3.0 and 3.3.1 where I presume the breaking change happened:

diff --git a/index.js b/index.js
index 921df02..afe6ccc 100644
--- a/index.js
+++ b/index.js
@@ -16,6 +16,7 @@ function watchify (b, opts) {
     var delay = typeof opts.delay === 'number' ? opts.delay : 600;
     var changingDeps = {};
     var pending = false;
+    var updating = false;
     
     var wopts = {persistent: true};
     if (opts.ignoreWatch) {
@@ -93,14 +94,20 @@ function watchify (b, opts) {
             watchFile(mfile, dep);
         });
     });
+    b.on('bundle', function (bundle) {
+        updating = true;
+        bundle.on('error', onend);
+        bundle.on('end', onend);
+        function onend () { updating = false }
+    });
 
     function watchFile (file, dep) {
         dep = dep || file;
         if (ignored) {
-          if (!ignoredFiles.hasOwnProperty(file)) {
-            ignoredFiles[file] = anymatch(ignored, file);
-          }
-          if (ignoredFiles[file]) return;
+            if (!ignoredFiles.hasOwnProperty(file)) {
+                ignoredFiles[file] = anymatch(ignored, file);
+            }
+            if (ignoredFiles[file]) return;
         }
         if (!fwatchers[file]) fwatchers[file] = [];
         if (!fwatcherFiles[file]) fwatcherFiles[file] = [];
@@ -119,6 +126,9 @@ function watchify (b, opts) {
     function invalidate (id) {
         if (cache) delete cache[id];
         if (pkgcache) delete pkgcache[id];
+        changingDeps[id] = true;
+        if (updating) return;
+        
         if (fwatchers[id]) {
             fwatchers[id].forEach(function (w) {
                 w.close();
@@ -126,13 +136,14 @@ function watchify (b, opts) {
             delete fwatchers[id];
             delete fwatcherFiles[id];
         }
-        changingDeps[id] = true
         
         // wait for the disk/editor to quiet down first:
         if (!pending) setTimeout(function () {
             pending = false;
-            b.emit('update', Object.keys(changingDeps));
-            changingDeps = {};
+            if (!updating) {
+                b.emit('update', Object.keys(changingDeps));
+                changingDeps = {};
+            }
         }, delay);
         pending = true;
     }
diff --git a/package.json b/package.json
index 163e411..9113a23 100644
--- a/package.json
+++ b/package.json
@@ -1,12 +1,12 @@
 {
   "name": "watchify",
-  "version": "3.3.0",
+  "version": "3.3.1",
   "description": "watch mode for browserify builds",
   "main": "index.js",
   "bin": "bin/cmd.js",
   "dependencies": {
     "anymatch": "^1.3.0",
-    "browserify": "^11.0.0",
+    "browserify": "^11.0.1",
     "chokidar": "^1.0.0",
     "defined": "^1.0.0",
     "outpipe": "^1.1.0",

[soryy708]

I just ran the test bench on the same versions range, but this time using requireq instead of import.

In 3.0.0 through 3.3.0 it says "Cannot find module 'someNonExistantPackage'" as it should. In all newer versions it freezes.

[soryy708]

So the workaround is to use version 3.3.0... which is 4 years old at this point.

[soryy708]

Looks like the root cause may be an issue with the implementation of a fix done years ago:

track updating status to remove race condition that deletes watchers

Commenting out b.on('bundle', function (bundle) { ... }) fixes this bug.
Now lets see how to fix it properly...

[soryy708]

The offending line of code is specifically bundle.on('error', onend);. If this is commented out, this bug stops happening.
Perhaps we're preventing browserify from handling the error properly by overwriting the callback instead of adding a new one?