hapijs / good

hapi process monitoring
Other
525 stars 161 forks source link

Fix leaking streams because monitor’s _dataStream is never closed #435

Closed rluba closed 8 years ago

rluba commented 8 years ago

I discovered this by accident in my tests: When creating and stopping hapi servers (for each test) with attached good plugin and using good-console as reporter, you’ll pretty soon see something like this:

(node) warning: possible EventEmitter memory leak detected. 11 close listeners added.

… because good-console pipes the monitor’s _dataStream (after some tranformation) into console.stdout, thereby attaching several event listeners to the latter. But the _dataStream is never closed, so the streams and event listeners remain attached to the global object.

I see two possible solutions:

  1. Either change good-console to
    1. store a reference to the transformed stream before piping into stdout,
    2. listen to the monitor’s stop event and
    3. when it occcurs: unpipe the transformed stream.
  2. or change good to EOF the _dataStream so that all piped-to streams are disconnected automatically.

This PR implements 2.

I’m not a big fan of the setTimeout workaround, but closing _dataStream right away causes problems when using extensions: [stop]: Its event handler also listens to the server’s stop event, receives it after the monitor and then tries to push into the closed _dataStream, causing an exception. (The monitor deregisters the extension event listeners, but the current stop event is already being processed.)

Instead of the setTimeout one could use a guard clause in the extension event listener but then the stop event would be dropped.

arb commented 8 years ago

Can you do something like this rather than the setTimeout?

https://github.com/hapijs/good/blob/v7.0.0/lib/monitor.js#L199-L202

Also, have you run npm test locally before submitting a PR? Your code is not passing lint.

arb commented 8 years ago

Also, I'm not 100% certain I want to make this change as it potentially has downstream impacts and I'm not really doing anything other than critical bug fixes for the 6.x.x line at present.

rluba commented 8 years ago
rluba commented 8 years ago

Interesting that c244d6d broke on the node4 / hapi 10 build. Might be a lovely race due to the setTimeouts (it passed for d3c5f20).

arb commented 8 years ago

If all you're trying to do is get your tests to stop yelling at you about the potential EventEmitter leak, you can run (just in your test code mind you)

process.stdout.setMaxListeners(0)
// OR
process.stdout.setMaxListeners(Infinity)
rluba commented 8 years ago

@arb Sure, but that just feels like an ugly workaround for the problem, not a real solution.

rluba commented 8 years ago

I’ve replaced the flaky setTimeout in the monitor test with a separate test that uses a latch for synchronization to avoid the race condition between the setTimeout in the implementation and the one in the test (without adding an artificial delay to the test).

arb commented 8 years ago

@rluba as I said before, I don't think I can merge this in safely at this point. I will admit that I 100% made a mistake in this version of good by NOT pushing null into the data stream and instead using the goofy "stop" event from good to tell streams to tear themselves down.

That being said, I can't say with any certainty what this would do to all of the good-reporters that might not be expecting null and are already in existence in the wild.

If the EventEmitter leak is what's causing you problems, try my suggested fix. I encourage you to follow along with the v7.0.0 branch of good. Maybe we can catch issues like this before it's released :smile:

rluba commented 8 years ago

That being said, I can't say with any certainty what this would do to all of the good-reporters that might not be expecting null and are already in existence in the wild.

@arb The reporters will never see a null data event! Pushing a null simply sets the EOF flag and listeners will receive an end event after reading all data. If they don’t handle it, they will work as before (but without leaking resources anymore).

rluba commented 8 years ago

@arb Would you merge this into the 7.0-Branch?

arb commented 8 years ago

I've already implemented this in the 7.0 branch, but there is plenty of other things to do in there.

https://github.com/hapijs/good/commit/dba94981ff0895b58320ae39f008e7c46ff2581a

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.