Stop the WebServer after running tests #928

Merged
merged 2 commits into from Mar 11, 2014

Conversation

Projects
None yet
3 participants
Contributor

jpommerening commented Feb 18, 2014

When we're done running the tests and all clients are disconnected, we should stop the HTTP server. This way another karma instance (or anything else for that matter) may reuse the port later.

For example, if we're using grunt-karma and have multiple successive Karma tasks we want to run, the output will look something like this (notice how karma consumes more and more ports):

$ grunt karma
Running "karma:part_one" (karma) task
INFO [karma]: Karma v0.10.9 server started at http://localhost:9876/
INFO [launcher]: Starting browser PhantomJS
INFO [PhantomJS 1.9.7 (Linux)]: Connected on socket -7kxMeoUxVJpOdRChLvW
PhantomJS 1.9.7 (Linux): Executed 55 of 55 SUCCESS (0.246 secs / 0.017 secs)
TOTAL: 55 SUCCESS

Running "karma:part_two" (karma) task
WARN [karma]: Port 9876 in use
INFO [karma]: Karma v0.10.9 server started at http://localhost:9877/
INFO [launcher]: Starting browser PhantomJS
INFO [PhantomJS 1.9.7 (Linux)]: Connected on socket hSO6q1EwCkDNwF5ohNQT
PhantomJS 1.9.7 (Linux): Executed 6 of 6 SUCCESS (0.193 secs / 0.006 secs)
TOTAL: 6 SUCCESS

Running "karma:part_three" (karma) task
WARN [karma]: Port 9876 in use
WARN [karma]: Port 9877 in use
INFO [karma]: Karma v0.10.9 server started at http://localhost:9878/
INFO [launcher]: Starting browser PhantomJS
INFO [PhantomJS 1.9.7 (Linux)]: Connected on socket sREEumil-I0NjY9ZhOiv
PhantomJS 1.9.7 (Linux): Executed 26 of 26 SUCCESS (0.249 secs / 0.011 secs)
TOTAL: 26 SUCCESS

This is most likely what is going on in karma-runner/grunt-karma#72.

To resolve this, I added a few lines in disconnectBrowsers() to call the http.Server.close() method. The final done callback is deferred until the server closed it's connections and released the port.

fix(web-server): close webserver after running
When we're done running the tests and all clients are disconnected,
we should stop the http server. This way another karma instance (or
anything else for that matter) may reuse the port later.
Contributor

jpommerening commented Feb 19, 2014

Also, remove listeners from process after running, to maybe leak a little bit less stuff…

(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.
Trace:
  at process.EventEmitter.addListener (events.js:160:15)
  at process.on.process.addListener (node.js:768:26)
  at start (/…/karma/lib/server.js:149:13)
  at [object Object].invoke (/…/karma/node_modules/di/lib/injector.js:75:15)
  at Object.exports.start (/…/karma/lib/server.js:211:12)
  at Object.<anonymous> (/…/grunt-karma/tasks/grunt-karma.js:46:14)
  at Object.<anonymous> (/…/grunt/lib/grunt/task.js:264:15)
  at Object.thisTask.fn (/…/grunt/lib/grunt/task.js:82:16)
  at Object.<anonymous> (/…/grunt/lib/util/task.js:282:30)
  at Task.runTaskFn (/…/grunt/lib/util/task.js:235:24)
  at Task.<anonymous> (/…/grunt/lib/util/task.js:281:12)
  at Task.<anonymous> (/…/grunt/lib/util/task.js:215:7)
  at [object Object]._onTimeout (/…/grunt/lib/util/task.js:225:33)
  at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

young commented Feb 20, 2014

Just ran into this issue as well. Thanks for the fix!

lib/server.js
- var disconnectBrowsers = function(code) {
+ // Keep track of listeners attached to the process to remove them later
+ var attachedListeners = {};
+ function removeProcessListeners() {
@vojtajina

vojtajina Mar 8, 2014

Contributor

Why can't we use process.removeAllListeners?

@vojtajina

vojtajina Mar 8, 2014

Contributor

Oh, nvm, because there might be other apps listening on this process...

Contributor

vojtajina commented Mar 8, 2014

@jpommerening this is great! thank you!

Just one refactoring idea:
How about defining processWrapper that remembers all the listeners and then processWrapper.removeAllListeners rememoves them.... so that the code in server does not have to deal with it...

Contributor

jpommerening commented Mar 8, 2014

Great idea!

@vojtajina vojtajina self-assigned this Mar 8, 2014

Contributor

jpommerening commented Mar 9, 2014

Like that?

I wanted to add a test to the server.spec.coffee to make sure it closes the port, but I don't know how to get started.

lib/server.js
+ log.debug('%s failed to capture, aborting the run.', browser);
+ disconnectBrowsers(1);
+ });
+ }
@jpommerening

jpommerening Mar 9, 2014

Contributor

Uhm. I can't remember adding this if block. How did it get there?

lib/emitter_wrapper.js
@@ -0,0 +1,48 @@
+module.exports = function wrap(emitter) {
+
+ function Emitter() {
@vojtajina

vojtajina Mar 10, 2014

Contributor

I think it would be better to have EmitterWrapper not to inherit Emitter. This only gives you fake API which does not work as expected (calling anything but the methods you defined would call that on the wrapper, not wrapped emitter).

@jpommerening

jpommerening Mar 10, 2014

Contributor

Ohh, you're right.

lib/emitter_wrapper.js
+ return this;
+ };
+
+ this.removeListener = function (event, listener) {
@vojtajina

vojtajina Mar 10, 2014

Contributor

You don't use this method, right? Why bother then? I would only implement the stuff we actually need...

@jpommerening

jpommerening Mar 10, 2014

Contributor

Putting it in it's own file I felt it was a ”good idea" to expose an EventEmitter-like API. I guess I could just remove it.

Contributor

vojtajina commented Mar 10, 2014

@jpommerening Yeah, the code in server.js is basically "untested mess". I mean, it is tested with the e2e tests;-)

I think, extracting some of the logic into the EmitterWrapper is a good solution - we can easily unit test that. For this PR, I would be fine with just that.

Then, we can extract more of this "mess" into separate objects and unit test that (basically what you did with the wrapper, do it with other logic in server.js).

Also, we could e2e test it - eg. run Karma using the API and then check whether the listeners were removed; test whether the webserver port has been released and all this stuff. In order to make this easier, we need to merge #684 first, as the current e2e testing is not flexible enough...

Contributor

jpommerening commented Mar 10, 2014

Alright; I removed the unused function and fixed the inheritance thing. Would you like me to rebase this?

I'll try to keep an eye on #684 and come up with the tests when I've got some spare time, ok?

Contributor

vojtajina commented Mar 10, 2014

This looks great, can you squash it into two commits (1/ close webserver, 2/ remove process listeners)

Thanks @jpommerening!

@vojtajina vojtajina added this to the v0.12 milestone Mar 10, 2014

fix(web-server): detach listeners after running
Remove listeners from the global process and the webserver when we're
done. The EmitterWrapper can be used to wrap any EventEmitter and
remembers the listeners that were via the wrappers addListener/on
function. Then the wrapper's removeAllListeners function can be used
to remove the wrapper's listeners, leaving the listeners that were
added directly to the unwrapped emitter attached.
Contributor

jpommerening commented Mar 10, 2014

Done 👍

vojtajina added a commit that referenced this pull request Mar 11, 2014

@vojtajina vojtajina merged commit 0717862 into karma-runner:master Mar 11, 2014

Contributor

vojtajina commented Mar 11, 2014

Great! Thank you very much @jpommerening

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