Skip to content

Commit

Permalink
fix(websocket): fixes websocket upgrade issue (#82)
Browse files Browse the repository at this point in the history
  • Loading branch information
@chimurai
chimurai committed May 29, 2016
1 parent 9e542fa commit 53fe79b
Showing 1 changed file with 5 additions and 12 deletions.
17 changes: 5 additions & 12 deletions lib/index.js
View file
@@ -1,3 +1,4 @@
var _ = require('lodash');
var httpProxy = require('http-proxy');
var configFactory = require('./config-factory');
var handlers = require('./handlers');
Expand All @@ -10,7 +11,8 @@ var getArrow = require('./logger').getArrow;
module.exports = HttpProxyMiddleware;

function HttpProxyMiddleware(context, opts) {
var isWsUpgradeListened = false;
// https://github.com/chimurai/http-proxy-middleware/issues/57
var wsUpgradeDebounced = _.debounce(handleUpgrade);
var config = configFactory.createConfig(context, opts);
var proxyOptions = config.options;

Expand All @@ -28,10 +30,7 @@ function HttpProxyMiddleware(context, opts) {

// https://github.com/chimurai/http-proxy-middleware/issues/19
// expose function to upgrade externally
middleware.upgrade = function(req, socket, head) {
handleUpgrade(req, socket, head);
isWsUpgradeListened = true;
};
middleware.upgrade = wsUpgradeDebounced;

return middleware;

Expand All @@ -52,13 +51,7 @@ function HttpProxyMiddleware(context, opts) {
}

function catchUpgradeRequest(server) {
// make sure only 1 handle listens to server's upgrade request.
if (isWsUpgradeListened === true) {
return;
}

server.on('upgrade', handleUpgrade);
isWsUpgradeListened = true;
server.on('upgrade', wsUpgradeDebounced);
}

function handleUpgrade(req, socket, head) {
Expand Down

9 comments on commit 53fe79b

@julbra
Copy link

@julbra julbra commented on 53fe79b Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a leak for me with ws: true.
Every request seems to add an upgrade handler on the server. After 10 requests I get:

(node) warning: possible EventEmitter memory leak detected. 11 upgrade listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at Server.addListener (events.js:252:17)
    at catchUpgradeRequest (node_modules/http-proxy-middleware/lib/index.js:52:16)
    at middleware (node_modules/http-proxy-middleware/lib/index.js:47:13)

@chimurai
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share your server+proxy configuration?

@julbra
Copy link

@julbra julbra commented on 53fe79b Sep 21, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the server is browser-sync, I'm also hitting #15. I discovered the leak trying to debug what's going wrong there.

Here is my proxy config:

var proxy = require('http-proxy-middleware');
var proxyMiddleware = proxy('/pp', {
      ws: true,
      prependPath: false,
      target: formattedUrl,
      changeOrigin: true,
      logLevel: 'debug'
    });

And here is the server config:

var browserSync = require('browser-sync').create();
browserSync.init({
    server: {
      baseDir: "../dist",
      middleware: [proxyMiddleware]
    },
    ghostMode: false,
    open: false,
    port: 3100
  }, function () {
    callback();
  });

@chimurai
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @julbra.
As you already discovered, there is still no solution for browser-sync and websockets proxy.
Wondering if this memory leak issue is isolated to browser-sync or is it also reproducable in express or connect?

@chimurai
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested the example setup http-proxy-middleware/examples/websocket with 20 active connections without issues.

This issue is probably similar to #108

Would be great if you can create a minimal working setup with express or connect in which the issue is exposed. (http://stackoverflow.com/help/mcve)

@julbra
Copy link

@julbra julbra commented on 53fe79b Sep 26, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chimurai I created a minimal project that reproduces the bug in browser-sync:

Clone from https://github.com/julbra/http-proxy-middleware-leak-test.git
npm install then run gulp
wget http://localhost:3100/ 10 times
After the 10th wget I get the warning.

julien ~/workspace/http-proxy-middleware-leak-test (master=) $ gulp
[15:58:37] Using gulpfile ~/workspace/http-proxy-middleware-leak-test/gulpfile.js
Example server listening on port 3000
[15:58:37] Starting 'default'...
[HPM] Proxy created: /  ->  http://localhost:3000
[HPM] Subscribed to http-proxy events:  [ 'error', 'close' ]
[BS] Access URLs:
 -------------------------------------
       Local: http://localhost:3100
    External: http://xxx.xxx.xxx.xxx:3100
 -------------------------------------
          UI: http://localhost:3001
 UI External: http://xxx.xxx.xxx.xxx:3001
 -------------------------------------
[BS] Serving files from: .
[15:58:37] Finished 'default' after 76 ms
[HPM] GET / -> http://localhost:3000
[HPM] GET / -> http://localhost:3000
[HPM] GET / -> http://localhost:3000
[HPM] GET / -> http://localhost:3000
[HPM] GET / -> http://localhost:3000
[HPM] GET / -> http://localhost:3000
[HPM] GET / -> http://localhost:3000
[HPM] GET / -> http://localhost:3000
[HPM] GET / -> http://localhost:3000
[HPM] GET / -> http://localhost:3000
(node) warning: possible EventEmitter memory leak detected. 11 upgrade listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at Server.addListener (events.js:252:17)
    at catchUpgradeRequest (/home/julien/workspace/http-proxy-middleware-leak-test/node_modules/http-proxy-middleware/lib/index.js:51:16)
    at middleware (/home/julien/workspace/http-proxy-middleware-leak-test/node_modules/http-proxy-middleware/lib/index.js:46:13)
    at call (/home/julien/workspace/http-proxy-middleware-leak-test/node_modules/connect/index.js:239:7)
    at next (/home/julien/workspace/http-proxy-middleware-leak-test/node_modules/connect/index.js:183:5)
    at next (/home/julien/workspace/http-proxy-middleware-leak-test/node_modules/connect/index.js:161:14)
    at next (/home/julien/workspace/http-proxy-middleware-leak-test/node_modules/connect/index.js:161:14)
    at respModifierMiddleware (/home/julien/workspace/http-proxy-middleware-leak-test/node_modules/resp-modifier/index.js:80:9)
    at call (/home/julien/workspace/http-proxy-middleware-leak-test/node_modules/connect/index.js:239:7)
    at next (/home/julien/workspace/http-proxy-middleware-leak-test/node_modules/connect/index.js:183:5)

Will test using two express instances to see if the bug is still there without browser-sync

@julbra
Copy link

@julbra julbra commented on 53fe79b Sep 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chimurai I updated https://github.com/julbra/http-proxy-middleware-leak-test with an express only task.
The same bug is also found there without browser-sync involved.

@chimurai
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @julbra.

Good news. Think I've found the issue.
I created a fix in PR #114. (Also created a separate issue #113 to track the issue)

Can you verify if this fixes the memory leak? (You only need to replace 1 file.)

@julbra
Copy link

@julbra julbra commented on 53fe79b Sep 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chimurai fixes the bug, ship it ;)

Please sign in to comment.