nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript πŸš€
https://nestjs.com
MIT License
66.89k stars 7.56k forks source link

Multiple @WebSocketGateway ends up with `Error during WebSocket handshake` #271

Closed maxime1992 closed 6 years ago

maxime1992 commented 6 years ago

I'm submitting a...


[ ] Regression 
[ x ] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Within my app, I was initially managing socket connection from only one file. Everything was working great.

Then, I decided to start some code refactoring and split this file in 2: One for users, one for orders.

@WebSocketGateway({ namespace: 'users' })
export class UsersService implements OnGatewayDisconnect {
  ...
}

and

@WebSocketGateway({ namespace: 'orders' })
export class OrdersService implements OnGatewayConnection {
  ...
}

As soon as I do that, I end up with an error displayed in the frontend: WebSocket connection to 'ws://localhost:3000/socket.io/?EIO=3&transport=websocket&sid=some-sid' failed: Error during WebSocket handshake: Unexpected response code: 400

I can start my app with just the user part, and when I comment everything related to websockets within orders, it's working.

Expected behavior

I'd like to be able to use multiple @WebSocketGateway for code organisation.

Minimal reproduction of the problem with instructions

Unfortunately I don't have time to create a repro right now. So in order to reproduce, create a project with 2 @WebSocketGateway and see what's happening in frontend side.

Just let me know if a repro would be super useful and I'll do my best to help.

What is the motivation / use case for changing the behavior?

I think it's a bug, but maybe it's just something I haven't understand with websockets? Also, I've been putting here some example with a namespace but it wasn't working either without namespace.

Environment


Nest version: 4.2.2

For Tooling issues:
- Node version: 8.6.0
- Platform: Ubuntu 17.10, 64 bits

Others:

Yes! It's my first project with Nest and also my first issue.
So I'm taking few lines to say thank you for this amazing project, I do feel very productive with it :smile:

Congrats and keep up the good work :clap: :clap: :clap:

kamilmysliwiec commented 6 years ago

Hi @maxime1992, Thanks! πŸ™‚ Breaking your gateway into several classes should work fine without any troubles. I'm not sure if the issue corresponds to Nest. It's either sth with your server configuration πŸ™

maxime1992 commented 6 years ago

Umh ok then, I'll give another look to my code and if I can't find anything, I'll try to make a small repro!

maxime1992 commented 6 years ago

@kamilmysliwiec I've pushed my current branch, which is a huge refactor of a backend to use NestJs :)

You might want to take a look into this commit: https://github.com/maxime1992/pizza-sync/pull/48/commits/8fc6d309b45acbfdf55e6c645d725c8b4eb76534

The namespace "user" is fine: https://github.com/maxime1992/pizza-sync/pull/48/commits/8fc6d309b45acbfdf55e6c645d725c8b4eb76534#diff-d76db01f9d25083267cf415b1503dd72R15

But the orders namespace... Just switching from @Component to @WebSocketGateway is giving me the error: https://github.com/maxime1992/pizza-sync/pull/48/commits/8fc6d309b45acbfdf55e6c645d725c8b4eb76534#diff-6f8fae269096b88da9f7cb11d7291891R13

I know it's not really pleasant to dig into some code for debug, so I'm just sharing that while I'm creating a smaller reproduction of the problem (if any...)

maxime1992 commented 6 years ago

@kamilmysliwiec I managed to make a repro of the bug on a minimal project.

Here it is: https://github.com/maxime1992/nestjs-repro-bug-multiple-websocketgateway/commits/master

Take a look to 2 latest commits:

How to repro:
git clone https://github.com/maxime1992/nestjs-repro-bug-multiple-websocketgateway.git
cd nestjs-repro-bug-multiple-websocketgateway

First terminal:

yarn
nodemon index.js

Second terminal:

cd frontend
yarn
http-server

(if you don't have http-server: yarn global add http-server) Go to http://127.0.0.1:8080 and take a look to your console + the code.

maxime1992 commented 6 years ago

@kamilmysliwiec could you give a look into the minimal repro? I'm really lost here... If you see anything wrong on the minimal repro I'd be glad to know and it'd probably help on my bigger project :) Thanks

FriOne commented 6 years ago

@maxime1992 I guess it is related to http-server. https://github.com/ArekSredzki/electron-release-server/issues/40#issuecomment-226353637

maxime1992 commented 6 years ago

@FriOne thanks, but the same happens with angular-cli (and I'm not sure whether it uses http-server or not under the hood, but I think that it's a webpack server).

But this is a track we have to follow :)

hpop commented 6 years ago

Ran into the same issue today as well. I have two WebSocketGateway's and both of them are working fine individually. As soon as I add them both to the module, one stops working and I am seeing the Unexpected response code: 400 Error.

maxime1992 commented 6 years ago

Hallelujah! I thought I was the only one and I was going mad. I started to rewrite those parts in a different way (uglier one...) but I really hope to keep the code the way I want (with 2 WebSocketGateways).

BorntraegerMarc commented 6 years ago

Is there an update/ETA on when we can expect a fix for this bug?

kamilmysliwiec commented 6 years ago

Hi guys, Thanks for your patience! It's a bug, I have found a reason already. It'll be fixed in the nearest update πŸŽ‰

cojack commented 6 years ago

Waiting for this too, can't wait <3

maxime1992 commented 6 years ago

Same! Even if it's fixed before the release I'll just build Nest locally :sweat_smile: It's the last thing I'm waiting for before release the huge update of Pizza-Sync with a full rewrite of the backend, using Nest :tada: :smile:

kamilmysliwiec commented 6 years ago

Please update your packages into 4.5.0 and let me know whether it fixes your issue or not πŸ™‚

maxime1992 commented 6 years ago

THANK YOU.

So I've been trying it very quickly and it seems to be working (need a little bit more investigation tho).

That said, I might be wrong but I think that when you have a namespace there's still an error.

So instead of having 2 namespaces like that:

I just define the 2 WebSocketGateway without namespace and so far it seems to work!

Thanks again @kamilmysliwiec

kamilmysliwiec commented 6 years ago

@maxime1992 thank you for your watchfulness! it's fixed in 4.5.1 (quick fix heh)

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.