fix(server): Remove Socket.IO listeners

[briandipalma]

Not removing Socket.IO listeners can cause memory issues
in long running processes that spawn many Karma instance.

Fixes #2980

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[briandipalma]

I signed it! I signed this last year so not sure why it says I haven't...I guess my email address is different?

[dignifiedquire]

thanks for finding this

[briandipalma]

The Travis CI failure seems odd, it's only failing in node 8 but it's complaining about the commit message. Surely that should fail in all node envs if that's incorrect?

[briandipalma]

The AppVeyor failure was present before my changes (I ran the build before making any changes).

1) middleware.proxy
       should bind proxy event:
     AssertionError: expected [ [Function: proxyReq] ] to equal true
      at Context.<anonymous> (test\unit\middleware\proxy.spec.js:349:71)

Should I look at it or is a well know failure?

[johnjbarton]

Seems like the Travis was run against c1cd5cb which is a merge.

Please try pulling the new upstream head and rebasing your change. I bet that fixes it.

[briandipalma]

I signed it!

[googlebot]

CLAs look good, thanks!

[briandipalma]

OK, the Travis failure is fixed and I signed the CLA with my other account so there's just the AppVeyor failure to fix.

[briandipalma]

It looks to me like the AppVeyor build is running on node.js 4 which doesn't fully support let. It's nothing to do with my changes that I can see. Does Karma still support node.js 4? Will I remove it from the appveyor.yml file?

[johnjbarton]

Sorry the package.json still says

"engines": {
    "node": ">= 4"
  },

In the past changing the node support triggered a major version number update. So I think the short path is to add "use strict" to your code to allow the let

[briandipalma]

I don't think it's my code that's causing the failure, it's npm?

https://ci.appveyor.com/project/dignifiedquire/karma/build/1705/job/30vsaqw8pw30gmta

npm --version
C:\Users\appveyor\AppData\Roaming\npm\node_modules\npm\bin\npm-cli.js:79
      let notifier = require('update-notifier')({pkg})
      ^^^
SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode

In the AppVeyor yml file npm is upgraded

  # Upgrade npm
  - npm install -g npm

as part of the setup. I wonder if it ends up installing a version of npm that's not compatible with node4? I can remove the upgrade of npm from the AppVeyor yml file?

[johnjbarton]

We should use a fixed version of npm that matches the lowest nodejs we support (4). I guess
npm install -g npm@3

[briandipalma]

From what I can tell node.js 4 is shipped with npm 2, https://nodejs.org/en/download/releases/ I presume that's the latest npm it supports. The node.js 4 version installed during the tests is one of the latest (4.8.7) and it would naturally ship with an up to date npm for its node version (2.15.11).

I feel removing the npm upgrade step might be the safest initial step. I'll try that and see what happens.

[briandipalma]

OK that progresses the build further so the UTs start running but I see a failure in the UTs that was there from before my changes. I debugged it and it looks to me like the failing UT is incorrect. The UT was added in this small PR #2596

The original PR didn't seem to have a green build and was merged regardless, unless I'm misunderstanding the conversation in the PR?

The lines which I believe are incorrect in the test are these

expect(parsedProxyConfig[0].proxy.listeners('proxyReq', true)).to.equal(true)
expect(parsedProxyConfig[0].proxy.listeners('proxyRes', true)).to.equal(true)

The listeners method is this method https://nodejs.org/api/events.html#events_emitter_listeners_eventname on EventEmitter which returns <Function[]> and not bool. So I'm changing those two lines to:

 expect(parsedProxyConfig[0].proxy.listeners('proxyReq')[0]).to.equal(config.proxyReq)
 expect(parsedProxyConfig[0].proxy.listeners('proxyRes')[0]).to.equal(config.proxyRes)

@dignifiedquire @kyo-ago does that seem reasonable?

[briandipalma]

OK the build is all green, for what it's worth the test added in this small PR fails for me locally https://github.com/karma-runner/karma/pull/2595/files I think it's assuming that trying to call toString() on a Proxy will always throw a TypeError but it doesn't seem to in the latest Chrome, it does in Fx.

Test it with

new Proxy(function abc (a, b, c) { return 'whatever' }, {}).toString()

In the console. I've left that UT alone for the moment but it might fail in future if the browser the tests are running against changes.

[johnjbarton]

Sorry Brian we're blocked by #2951 now that Travis is running chrome 61.

[johnjbarton]

@briandipalma please sync up and retest, master now builds for me.

[briandipalma]

@johnjbarton Done, I kept the removal of the npm upgrade step in the build as the node.js deployed by AppVeyor seems to come with the latest npm available in the official node.js releases (https://nodejs.org/en/download/releases/) so I have a feeling that those are the "safe/approved" npm versions per node.js version.

[johnjbarton]

Thanks @briandipalma , would you please separate the appveyor and proxy fixups into their own PRs? I don't want the discussion of completely different issues to be all tangled up. (We typically squash all commits for a PR for this same reason).