chimurai / http-proxy-middleware

:zap: The one-liner node.js http-proxy middleware for connect, express, next.js and more
MIT License
10.71k stars 834 forks source link

TypeError: Cannot read property 'on' of null at catchUpgradeRequest #143

Open tonygentilcore opened 7 years ago

tonygentilcore commented 7 years ago

Expected behavior

Able to proxy websockets with webpack 2.

Actual behavior

Not able to proxy websockets with webpack 2.

Setup

webpack-dev-server v2.2.1 was just pushed to stable. It depends on http-proxy-middleware ~0.17.1, which resolves to 0.17.3.

At the previous stable version, v1.16.2, this webpack config works:

  devServer: {
    https: true,
    proxy: {
      '/my-backend': {
        changeOrigin: true,
        pathRewrite: {'^/my-backend': ''},
        target: myBackendUrl,
        ws: true,
      },
  }

At v2.2.1, it does not. Instead I get:

TypeError: Cannot read property 'on' of null
    at catchUpgradeRequest (/work/my-project/node_modules/http-proxy-middleware/lib/index.js:56:19)
    at middleware (/work/my-project/node_modules/http-proxy-middleware/lib/index.js:48:13)
    at /work/my-project/node_modules/webpack-dev-server/lib/Server.js:223:15
    at Layer.handle [as handle_request] (/work/my-project/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/work/my-project/node_modules/express/lib/router/index.js:312:13)
    at /work/my-project/node_modules/express/lib/router/index.js:280:7
    at Function.process_params (/work/my-project/node_modules/express/lib/router/index.js:330:12)
    at next (/work/my-project/node_modules/express/lib/router/index.js:271:10)
    at middleware (/work/my-project/node_modules/http-proxy-middleware/lib/index.js:43:13)
    at /work/my-project/node_modules/webpack-dev-server/lib/Server.js:223:15
    at Layer.handle [as handle_request] (/work/my-project/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/work/my-project/node_modules/express/lib/router/index.js:312:13)
    at /work/my-project/node_modules/express/lib/router/index.js:280:7
    at Function.process_params (/work/my-project/node_modules/express/lib/router/index.js:330:12)
    at next (/work/my-project/node_modules/express/lib/router/index.js:271:10)
    at goNext (/work/my-project/node_modules/webpack-dev-middleware/middleware.js:28:49)
chimurai commented 7 years ago

WDS 1.16.2 seems to be using HPM 0.17.1 (WDS package.json)

@tonygentilcore, can you verify if the issue disappears when you set the http-proxy-middleware explicitly to 0.17.1 in your WDS 2.2.1 setup?

chimurai commented 7 years ago

Overview of code changes v0.17.1...v0.17.3

tonygentilcore commented 7 years ago

So, actually, both WDS major versions resolve to the same HPM version, 0.17.3 (because they depend on ~0.17.1 which allows the patch version to float).

Working:

└─┬ webpack-dev-server@1.16.2 
...
  ├─┬ http-proxy-middleware@0.17.3 
  │ ├─┬ http-proxy@1.16.2 
  │ │ ├── eventemitter3@1.2.0 
  │ │ └── requires-port@1.0.0 
  │ └─┬ is-glob@3.1.0 
  │   └── is-extglob@2.1.1 
...

Not working:

└─┬ webpack-dev-server@2.2.1 
...
  ├─┬ http-proxy-middleware@0.17.3 
  │ ├─┬ http-proxy@1.16.2 
  │ │ ├── eventemitter3@1.2.0 
  │ │ └── requires-port@1.0.0 
  │ └─┬ is-glob@3.1.0 
  │   └── is-extglob@2.1.1 
...

But just to be certain, I did try WDS 2.2.1 w/ HPM 0.17.1. Same stack.

SpaceK33z commented 7 years ago

Interesting. A large change in webpack-dev-server was that we now use HTTP/2. For that, it uses the spdy package (confusing name, but it also provides HTTP/2 support). Because http/2 only works with https, it's only activated when you use https: true. So it could be that the spdy package and http-proxy-middleware are not compatible with each other. I'm going to try to reproduce this and report back.

SpaceK33z commented 7 years ago

I reproduced it, and found out that the spdy package does not set req.connection.server, which http-proxy-middleware depends upon.

chimurai commented 7 years ago

Worth investigating the spdy setup. Thanks for the background info @SpaceK33z .

With the ws: true option, HPM relies on a initial (GET) request to get the server object, so HPM can subscribe to the server's upgrade event.

In a really simplified setup, the server object is retrieved via:

middleware(req, res, next) {
   catchUpgradeRequest(req.connection.server);
}

source: https://github.com/chimurai/http-proxy-middleware/blob/8689cb429577a7722b9334e702535cb752191c43/lib/index.js#L48


Edit: You're fast @SpaceK33z :-D

Can you find a way to get the server object from the request? So we can listen to the upgrade event:

server.on('upgrade', upgradeHandler);
SpaceK33z commented 7 years ago

@chimurai I filed a bug report in spdy: https://github.com/spdy-http2/node-spdy/issues/303. I'll take a look if there is another way to get the server object.

chimurai commented 7 years ago

Great! Will also take a look in getting the server object. (Not sure if the mechanics are the same in http2 to establish a WebSocket connection)

chimurai commented 7 years ago

@SpaceK33z @tonygentilcore Some sad news:

NO WEBSOCKETS OVER HTTP/2

https://daniel.haxx.se/blog/2016/06/15/no-websockets-over-http2/ https://github.com/spdy-http2/node-spdy/issues/224#issuecomment-139122247


Also some good news: You can force spdy to use http/1.1; The server object will be available again in the request.

https://github.com/webpack/webpack-dev-server/lib/Server.js#L352

options.https.spdy = {
    // protocols: ["h2", "http/1.1"]
    protocols: ["http/1.1"]
};

This should hopefully fix the issue.

tonygentilcore commented 7 years ago

Nice, I can confirm that this config gets me going again!

  devServer: {
    https: {
      spdy: {
        protocols: ['http/1.1'],
      },
    },
    proxy: {
      '/my-backend': {
        changeOrigin: true,
        pathRewrite: {'^/my-backend': ''},
        target: myBackendUrl,
        ws: true,
      },
    },
  }
SpaceK33z commented 7 years ago

@chimurai noo that's a shame :(. Thanks for finding that out though. I could workaround this with a huge hack by checking if ws: true is used and if so, revert to http/1.1 (insert evil laugh 😈 )

chimurai commented 7 years ago

It's a shame indeed.

Glad to hear there is a workaround for this issue. (Thanks for sharing the WDS solution @tonygentilcore)

Perhaps I can catch the error and log an error so it'll be more developer friendly. Until http2 gets support for WebSockets there is little we can do about this.

blake-newman commented 7 years ago

@tonygentilcore @SpaceK33z @chimurai. By placing the spdy created server on the req.connection.server manually during express middleware execution this solved the problem for me. This is only possible if you are using node api and is a bit of a hack.

module.exports = function proxyCreator(app, server) {
  const wsProxy = proxyMiddleware('/socket.io', config);
  server.on('upgrade', wsProxy.upgrade);
  app.use((req, res, next) => {
    req.connection.server = server;
    wsProxy(req, res, next);
  });
};
lanceon commented 7 years ago

Replacing the devServer option

{
  ...
  https: true
  ...
}

with

...
https: {
  spdy: {
     protocols: ['http/1.1'],
   }
}
...

worked for me.

Thank you!

dherges commented 7 years ago

This seems also be related to the order of proxy configs:

BREAKS with Cannot read property 'on' of null at catchUpgradeRequest lib\index.js:63:19:

{
  "/notification/channel": {
    "target": "wss://foobar.local/notification/channel",
    "secure": false,
    "ws": true
  },
  "/something.json": {
    "target": "https://foobar.local/something.json",
    "secure": false
  }
}
TypeError: Cannot read property 'on' of null
    at catchUpgradeRequest (node_modules\http-proxy-middleware\lib\index.js:63:19)
    at middleware node_modules\http-proxy-middleware\lib\index.js:55:13)
    at node_modules\webpack-dev-server\lib\Server.js:224:15`

WORKS:

{
  "/something.json": {
    "target": "https://foobar.local/something.json",
    "secure": false
  },
  "/notification/channel": {
    "target": "wss://foobar.local/notification/channel",
    "secure": false,
    "ws": true
  }
}

Code in questions: lib/index.js#L46-L49

I think the if(proxyOptions.ws === true) should probably be moved to inner part of if(shouldProxy(...))

chimurai commented 7 years ago

Hi @dherges,

Thanks for diving into this issue.

Can you apply your suggested change locally in you node_modules folder? And see if this indeed fixes the issue?

dherges commented 7 years ago

Hi @chimurai,

sure, we have a repro in the office at my employer, w/ wss:// server. Will check that tomorrow morning.

It also affects Angular CLI ng users, since they (angular team) are pulling in webpack which pulls in http-proxy-middleware 😸

dherges commented 7 years ago

Hi @chimurai,

if moving the call to catchUpgradeRequest(..) to inner if(shouldProxy(...)), it does not handle websocket upgrade at all. Changing the code was no solution to us. Changing the order of proxy configs is working for us.

I cannot exactly tell when it happens that req.connections.server is not initialized and has an undefined value. It seems to happen when we connect to SSL backends w/ self-signed certs (wss:// and https:// targets + "secure": false). It works fine, when connecting to ws:// and http:// targets.

chimurai commented 7 years ago

Too bad the fix didn't work...

Are you using the latest version of webpack-dev-server? And can you share your devServer config?

dherges commented 7 years ago

Hi, we're using thru @angular/cli 1.0.2 default set up ... what they pull in. Would need to look at exact version next week in office.

Cheers, David

On 12. May 2017, at 17:33, chimurai notifications@github.com wrote:

Too bad the fix didn't work...

Are you using the latest version of webpack-dev-server? And can you share your devServer config?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

SomeoneIsWorking commented 2 days ago

I have this issue using bun but not node, I'm not using webpack dev server also, I'm using this library directly, latest version.