dev-cdn: one watcher per file #4

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

cricri commented May 26, 2012

When lots of bundles depend on the same files, node complains with the message
(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.
because each bundle declare a listener on each file.

To prevent this, we can use a unique watcher for each file, which notifies to all interested bundles the change.

Contributor

paulbaumgart commented Jun 20, 2012

Hey Christophe,

Thanks for the pull request, but I'm not going to merge this:

  1. It's a very minor bug in that it doesn't harm program operation: it just spits out garbage to the console. It's not a significant enough problem to justify increasing the complexity of the jsbundle code.
  2. The fact that this error message exists combined with the fact that you can't change the max listeners for StatWatchers is a bug in the Node API itself, IMO. If you don't like the fact that this error message gets printed, I suggest mentioning it on the Node mailing-list.

I hope my rationale makes sense to you. Thanks again for taking the time to write this patch.

Cheers,
Paul

Contributor

cricri commented Jun 20, 2012

Hey Paul,
I dont like this error message because I dont like to have useless error messages in my automated build process logs. It's only junk, and a pollution that might hide important (and true) errors.
Furthermore I dont like to have multiple watchers on the same event, when I can have one watcher notifying all the interested guys; it's not very efficient, and scalable.
Sadly, I dont see what I could simplify in the code to change your opinion (but I would do if you give me some hint and if it can make this pull request more merge-able)...
And sorry for the tabs in the source code :-(

Contributor

paulbaumgart commented Jun 20, 2012

I don't see how it matters for scalability, but I agree the error message is annoying. Here's a patch that might help:

diff --git a/bin/devcdn b/bin/devcdn
index 8ee958e..79c4c17 100755
--- a/bin/devcdn
+++ b/bin/devcdn
@@ -1,5 +1,14 @@
 #!/usr/bin/env node

+// disable unnecessary error output when watching a file from many bundles
+var err = console.error;
+console.trace = function() {};
+console.error = function(msg) {
+  if (!/warning: possible EventEmitter memory leak detected/.test(msg)) {
+    err.apply(this, arguments);
+  }
+};
+
 var argv = require('optimist').argv;
 var connect = require('connect');
 var fs = require('fs');
Contributor

paulbaumgart commented Jun 20, 2012

Alright, pushed a fix that suppresses that warning in a fairly clean way.

Contributor

cricri commented Jun 21, 2012

thanks paul

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