clean up after event listeners

[matthewmueller]

if you're running these things concurrency, it's pretty easy to run into:

(node) warning: possible EventEmitter memory leak detected. 11 uncaughtException listeners added. Use emitter.setMaxListeners() to increase limit.

[souly1]

+1 for the issue

[LarryBattle]

@matthewmueller Could you provide a test example?

[FredXue]

in my case, after adding a .wait(selector) action into the test, it runs into this situation. Any idea? or solution?

[LarryBattle]

The fix/hack is to add process.setMaxListeners(0); at the top of your script.
Source: http://stackoverflow.com/questions/9768444/possible-eventemitter-memory-leak-detected

To reproduce the warning message, remove process.setMaxListeners(0); from test/index.js then run the tests.

I wonder if there is actually a memory leak going on ❓

[philhaz]

process.setMaxListeners(0); is making no difference here - definitely seems related to the use of wait().

mocha 2.3.3, node 4.1.2, nightmare 2.0.7

[pcalin]

Same problem here. process.setMaxListeners(0) did not make a difference.

[nightink]

+1 for the issue

[victorwpbastos]

+1 for the issue

[fr-]

It seems to me that using nightmare in a long running/concurrent setup is prone to many errors and can cause all kinds of hiccups.

Firstly, there are some things to consider when registering a global (process) event handler in the ctor of any class, but in this specific case it's pretty important to clean them up in all cases that can be considered a shutdown/dtor scenario for nightmare:

1. Shutdown via end()
2. Exit of child process
3. Inability to start child process
4. Exceptional exit

This also applies for some of the control flow, which can cause nightmare to hang if the electron process abnormally quits, as it does not reject in those cases (mainly never returning from continue).

I have created a preliminary feature branch on my fork of nightmare that tries to tackle these, but it's not cleaned up. Basically it tries to achieve multiple things:

1. Clean up after event listeners
2. Dynamically increase the maximum event listeners when more instances of nightmare are created on a single process
3. Handle abnormal exists (such as electron SIGSEGV/SIGBUS, exit code != 0, exceptions)
4. Set a failed state on the nightmare instance along the lines of initial and ready in case electron crashes/exits abnormally

I am not yet convinced this is the best solution, so I am not yet issuing a pull request, but you can take a look here: fr-@c22b45c

It does seem to work for largely concurrent runs in my test cases, though. Any input appreciated!

[Namek]

My research: as @fr stated it's about a long test run but I believe it's related to number of tests rather than physical time (to be specific). In my case commenting one .click(..., ...) or .type() made it work without warning, but uncommenting any one of these made the warning occur again - tested many times. Adding .wait(100) doesn't bring the warning. What's more interesting (another bug probably), doing .wait(100).wait(100) hangs up.

By looking into https://github.com/segmentio/nightmare/blob/master/lib/actions.js I can see there's one common thing between click() and type() and not for wait() - it's a call to this._evaluate which can be found here: https://github.com/segmentio/nightmare/blob/master/lib/nightmare.js I can see only a call to once in there. Not sure what's to be cleaned here, do you have some ideas about it, @fr?

[fr-]

@Namek from what I can see there are multiple issues at the same time that also but not exclusively manifest themselves in warnings about about too many registered uncaughtException event listeners:

1. The most obvious one is of course, that the event listeners are registered on process (which is global) whenever a new Nigthmare instance is created and them not being removed after an instance is shut down.
2. Additionally there is a default maximum for listeners on any EventEmitter (10 is the default, as per: https://nodejs.org/api/events.html#events_emitter_setmaxlisteners_n) which can be increased (and depending on the node version queried) as need be. So in the legitimate case where more than 10 instances of Nightmare are created concurrently this number has to be increased with the number of instances. (as a side note: as there could be more code apart from nightmare that registers handlers on process one should also track the globally registered event listeners not created by nightmare).

The above points are being handled by the feature branch I posted, with the slight caveat that the meaning of shutdown is a matter of definition, but can be classified as in my previous comment.

The second part of the issue is handling abnormal exits and failure to start the underlying electron process. In these cases one also needs to ensure that none of the actions that are waiting on continue will hang forever when the underlying electron and therefore the IPC has shut down and will never emit said event. My proposed solution is to:

1. Return from continue immediately after an abnormal shutdown of electron
2. Set a failed state on the instance of nightmare such that failure can be detected
3. Clean up global resources (aka global event listeners in this case)

So coming back to _evaluate @Namek: one thing that just occurred to me is that the _evaluate function you point to could possibly wait for javascript indefinitely if the electron process crashes while handling the inbound javascript event. I'd have to review the code again to be sure, though.

In any case, as far as I can see there seem to be no more implications other than the aforementioned leading to the behavior described; but I haven't really had the time to dig any further, which is why I was looking for more input on the branch.

Btw: The wait(100).wait(100) problem might very well be another issue that possibly can be fixed with #320

As a general note: It does seem a bit unsafe to use once on the IPC as a control flow structure without handling exceptional cases...

[seep]

I'm also running into this, when using .wait(fn).

[billpull]

I have a pretty long running nightmare process that runs into this issue. it seemed fine until I got to the last method which involved setting and clicking a lot of form fields. I tried to move the code into just a couple of evaluate blocks to possibly keep the number of events down but that didn't seem to work.
https://gist.github.com/billpull/c8278fdf367455f4eb91

has anyone made any progress on this issue?

[EmiPhil]

One hack to get around the issue is to add

Emitter.defaultMaxListeners = 0;

to the ipc.js file in nightmare > lib.

Note that this is dangerous and could easily lead to memory leaks.

[mexius]

+1 for the issue

[jspri]

Still getting this using the latest version. Emitter.defaultMaxListeners = 0; in the ipc.js as recommended by @EmiPhil is the only way I could get these errors to stop, even using the latest version. process.setMaxListeners(0); did not work. I'm on win 10, node v5.5

[apimapper]

+1

[Oceanswave]

+1

[faylai]

+1

[glenchao-zz]

+1

[rickmed]

+1

[atwellpub]

+1

[casesandberg]

This was fixed with #407