Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Path not working on 2.x ws server #1000

Closed
cbratschi opened this issue Feb 13, 2017 · 12 comments
Closed

Path not working on 2.x ws server #1000

cbratschi opened this issue Feb 13, 2017 · 12 comments

Comments

@cbratschi
Copy link

So far the following code was working fine on 1.1.2:

    const ws       = require('ws');
    const WSServer = ws.Server;

    const wss = new WSServer({
        server: server,
        path: absPath,

        //do not use compression (slow & memory overhead)
        perMessageDeflate: false
    });

It uses a path and express connection. If the path is removed connections are established.

On 2.x the client just returns:

Error: unexpected server response (400)

It seems the path is not recognized and therefore the connection never being upgraded.

@lpinca
Copy link
Member

lpinca commented Feb 13, 2017

The value of the path option and the pathname of the connection URL must match.

@cbratschi
Copy link
Author

Somehow this longer works on 2.x. Could be something else but if I remove the path the client gets a response. Any ideas?

A different thing I discovered: the latest bufferutil and utf-8-validate modules are not compatible with ws 2.x. Which versions are exactly compatible?

@lpinca
Copy link
Member

lpinca commented Feb 14, 2017

If you remove the path option, path validation is not run so connection is always established unless there is another way to close it like verifyClient. Can you reduce the issue to a reproducible example?

ws@>=2.0.2 is compatible with all versions of bufferutil and utf-8-validate. ws@<2.0.2 only works with bufferutil@1 and utf-8-validate@<=2.

@lpinca
Copy link
Member

lpinca commented Feb 14, 2017

In ws@1 the connection was not closed when path validation failed but this is a bug as the request hangs in this case as it never receives a response.

In ws@2 when path validation fails the connection is correctly closed so I think this explains why you see a different behavior between the two versions.

@cbratschi
Copy link
Author

Well, I am registering several paths for the express based server. In my case the connection should stay open and be passed to the next ws handler until the path matches. If there is no match express should return a 404 error and close the connection.

So the shouldHandle() call should be done first before doing any further checks which could cause an abortConnection() call.

@lpinca
Copy link
Member

lpinca commented Feb 14, 2017

Express does not handle upgrade (WebSocket) requests so it will never answer with a 404 to an upgrade request. The Node.js HTTP server emits a 'request' event for non upgrade requests and an 'upgrade' event for upgrade requests. The express handler/router is only used when the 'request' event is emitted.

Hope this makes sense.

@cbratschi
Copy link
Author

I see.

If several paths are registered then the connection should only be closed if no matching path was found.

@lpinca
Copy link
Member

lpinca commented Feb 14, 2017

You can register only a single path in the WebSocket server.

@cbratschi
Copy link
Author

The main problem is that we are using several WebSocket servers with different paths but the same express server instance. This was working fine with ws 1.x. Now the first WebSocket server closes the connection if the path would match the path of the registered second WebSocket server. This is a crucial feature because we extend express to allow ws routes which register a WebSocket server using the route's path.

@lpinca
Copy link
Member

lpinca commented Feb 14, 2017

The main problem is that we are using several WebSocket servers with different paths but the same express server instance.

You didn't say anything about this! Take a look a this comment on how to make this work with ws@2.

@cbratschi
Copy link
Author

Thanks for the link. I refactored my code and now everything works again.

@lpinca
Copy link
Member

lpinca commented Feb 14, 2017

Awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants