MaxListenersExceedsWarning when used with http2

[akc42]

I am aware of #7 which was closed a while ago now. However the issue still is occurring for me, using node 7.10.0. I think that is probably because I am using the http2 as the server and because the app that it is serving is a polymer custom components one, so calling index.html does cause an immediate request for quite a large number of additional files (approx 40 if I remember correctly) which should be being served through serve-static.

I have put a trace on the warning and confirmed it comes from the on-finished module as expressed in #7. I have also puts some instrumentation in the module that is adding and removing the listeners, so I am pretty sure that although a lot are added they are all eventually removed again, to I don't think there is an actual memory leak happening.

I report it here to see if there is anything that serve static could do to prevent the warning occuring

[dougwilson]

Hi @akc42 I can definitely take a look! Can you provide me with an example I can see the issue so I can check it out in a debugger? Probably need at least all the following:

1. Version of Node.js
2. Version of this module and any other modules used in the code.
3. Complete code to run and debug.
4. Please include complete instructions on how to run the code, and how to make a request to it to cause the warning to print.

Just from the description, I'm not entirely sure there is anything that can be solved, since there isn't a memory leak. Do you have any thoughts on a possible solution off-hand?

[akc42]

@dougwilson to answer your points

1. 7.10.0 - as I said above

2. The key modules (there are many many more involved in the entire application) are

• "finalhandler": "^1.0.2",
• "http2": "^3.3.6",
• "serve-static": "^1.12.2",
• "router": "^1.3.0"

3. Difficult to provide, the complete application is massive and would be complex to understand. I don't want to publish it to the open internet because its confidential for a client, but I could provide access to a git repository if you send me a public ssh key, But in reality, the crux of the code is

const path = require('path');
require('dotenv').config({path: path.resolve(__dirname,'.env')});
const clientPath = path.resolve(__dirname,process.env.PAS_CLIENT_PATH);

const Router = require('router');
const serveStatic = require('serve-static');

const router = Router();

//... set up lots of other routes for ajax requests at the /api/... url

router.use('/', serveStatic(clientPath));

const server = require('http2').createServer(certs,async (req,res) => {
  const done = finalhandler(req,res);
  router(req,res,done);
});

As I said, the client side is Polymer application so it is full of html input lines which quickly result in large numbers of requests which need to be served statically. Just to illustrate how this quickly escalates index.html has <link rel="import" href="/src/pas-app.html" > and then /src/pas-app.html
has

<link rel="import" href="../bower_components/polymer/polymer.html">
<link rel="import" href="../bower_components/app-layout/app-header-layout/app-header-layout.html">
<link rel="import" href="../bower_components/app-layout/app-header/app-header.html">
<link rel="import" href="../bower_components/iron-a11y-keys/iron-a11y-keys.html">
<link rel="import" href="../bower_components/app-layout/app-scroll-effects/effects/waterfall.html">
<link rel="import" href="../bower_components/app-layout/app-toolbar/app-toolbar.html">
<link rel="import" href="../bower_components/iron-flex-layout/iron-flex-layout.html">
<link rel="import" href="../bower_components/paper-tooltip/paper-tooltip.html" >
<link rel="import" href="../bower_components/paper-icon-button/paper-icon-button.html" >
<link rel="import" href="../bower_components/paper-menu-button/paper-menu-button.html" >
<link rel="import" href="../bower_components/paper-menu/paper-menu.html" >
<link rel="import" href="../bower_components/paper-item/paper-item.html">
<link rel="import" href="../bower_components/lazy-imports/lazy-imports.html">
<link rel="import" href="../bower_components/iron-ajax/iron-ajax.html">

<link rel="import" href="pas-error.html">
<link rel="import" href="pas-session.html">
<link rel="import" href="pas-iconset.html">
<link rel="import" href="pas-utils.html">

and so on ...

What happens is, if the browser cache is clear and it is not loading these files from cache, then a massive spike of overlapping requests hits the middleware that server static creates for the router. and the event emitter warning is triggered.

It terms of a solution, my thoughts are that serve static could keep track of the number of parallel requests (by creating a static counter in the initial call to set up the middleware and then incrementing it just before send ing the stream in the middleware and listening for stream end event and decrementing it) (or maybe send should do this, but you wrote that also?). If it gets close the maxListeners then add some more temporarily.

[dougwilson]

Hi @akc42 sorry if I wasn't clear: it doesn't have to be your actual app, just an app. If you can't actually take some time to put together (3) and (4) so I can take a look, I think I have enough information to try to come up with an app and reproduction steps, but this is definitely on my back burner since that is going to add a lot of time I don't really have to figure this out. You're absolutely free to put together a pull request with a fix to get merged, of course !

As for your suggestion, I don't think that even makes sense. If we just keep incrementing the max listeners, then what is the purpose of even having a max at all? May as well simply disable the warning, right?

[akc42]

I'll try and put something together that replicates the problem but is simple. It will take me a short while though as I am busy for a few days.

[akc42]

@dougwilson
I am not sure of the correct protocol to follow here, but I hope this is useful. I made a repository with a simple application that demonstrates the problems

https://github.com/akc42/maxListenerTest

I think this answers your points 3 & 4

Re your response about not increasing max listeners. I don't agree with your assertion that there is no purpose to having max listeners if you just increase it. There is a purpose because other events in the system may actually be leaking listeners and you want to trap them. I am thinking that either serve-static or onfinished (probably the latter) knows what events it is creating so can increase the max for just those events. and not any others.

[dougwilson]

I don't agree with your assertion that there is no purpose to having max listeners if you just increase it.

Perhaps you misunderstood. I meant that in this case, on whoever the object is, it wouldn't have a purpose, since if I added one every time I added a listener, then if my code to remove the listener never runs (and thus decrements the max count), it would no longer catch the memory leak the warning is supposed to catch, effectively making it pointless to even try bumping the max. May as well just set the max to Infinity for whatever that object is at that point, turning the feature off. Of course, that means if any other module is also adding listeners to the same object, their leak would not be detected.

Complicating matters, this module support Node.js 0.8+ and there is no API in 0.8 (or 0.10 for that matter) to get the set max listeners, only set them, so just a single set to Infinity (disabling the feature on that object) is probably all that can be done. Of course, the max listeners setting applies to listeners or all types, so even in Node.js versions you can get the current value, the high limit will apply to all listeners on the object. This means that say I'm listening for the foo event 20 times. If I then set the limit to 20, it also increases the limit for the bar event, the baz event, etc. Node.js doesn't even have a method for this to just be limited to this module's usage.

I'm not sure if there is anything I can do. If you have and ideas, I would welcome a pull request, as right now the right solution is eluding me, and I'm not sure how to move forward.

Much appreciated!

[75lb]

@akc42 I don't suppose you could recommend a quick solution to make the warning go away?

[akc42]

My, rather simplistic solution (meaning I don't get all the extras from serve-static) was to use this to serve my static files

  //simple serve-static
  function staticFiles (cpath) {
    const clientPath = cpath;

    return (req, res, next) => {
      if (req.method !== 'GET' && req.method !== 'HEAD') {
        next();
        return;
      }
      let forwardError = false;
      let reqURL = url.parse(req.url).pathname;
      requestLog('send URL ', reqURL);
      const stream = send(req,reqURL,{root: clientPath});
      if (res.socket !== undefined) {
        res.socket.setMaxListeners(0);
      }
      stream.on('file', () => {forwardError = true;});//Real error after we have found file
      stream.on('error', err => {
        if (forwardError || !(err.statusCode < 500)) {
          next(err);
        } else {
          next();
        }
      });
      stream.pipe(res);
    };
  }

This function is then used as router middlewhere

      const router = Router();

      debug('Ask Manager to set up routes');
      manager.setRoutes(router);
      debug('Set up to serve static with possible transform');
      /*
       * I think the order of these next two middlewares are important
       * Both of them alter res.write and res.end to intersperse with their own
       * versions of these two routines.  If we are going to transpile we have
       * to do it before we compress the stream, so that goes first.
       */
      router.use('/', transformResponse(this.transformNeeded));
      //eslint-disable-next-line max-len
      //router.use(compression()); Temporarily disable as its sending headers which is effecting transpile.  Need to investigate
      //set up our static route
      router.use('/', staticFiles(clientPath));
      this._start(router);
    }