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

Remove path registry #885

Merged
merged 1 commit into from
Nov 9, 2016
Merged

Remove path registry #885

merged 1 commit into from
Nov 9, 2016

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Nov 6, 2016

The _webSocketPaths object is added to the underlying HTTP sever of a WebSocketServer when the path option is specified. It is used to store the path of each WebSocketServer using the same underlying HTTP server in order to avoid path conflicts.

const server = http.createServer();
const wss1 = new WebSocket.Server({ server, path: '/foo' });
const wss2 = new WebSocket.Server({ server, path: '/bar' });

This allows to use multiple WebSocket severs (one per path) and only one shared HTTP server.

The problem here is that each WebSocketServer adds a new listener for the upgrade event on the HTTP server and when that event is emitted, handleUpgrade is called on all servers.

Now, using the above example, a connection to ws://example.com/foo would first be established on wss1 and then closed by wss2 as they both write on the same socket.

Originally this worked because path validation did not close the connection on mismatch.

Passing the same HTTP server in the constructor when instantiating multiple WebSocket servers is not a good idea imo and should be avoided. I can't think of a use case where it makes sense.

In order to have multiple WebSocket servers and only one shared HTTP server, a developer could use something like this:

const wss1 = new WebSocket.Server({ noServer: true });
const wss2 = new WebSocket.Server({ noServer: true });
const server = http.createServer();

server.on('upgrade', (request, socket, head) => {
  const pathname = url.parse(request.url).pathname;

  if (pathname === '/foo') {
    wss1.handleUpgrade(request, socket, head, (ws) => {
      wss1.emit('connection', ws);
    });
  } else if (pathname === '/bar') {
    wss2.handleUpgrade(request, socket, head, (ws) => {
      wss2.emit('connection', ws);
    });
  } else {
    socket.destroy();
  }
});

@o0101
Copy link

o0101 commented Mar 24, 2017

Thanks for the great workaround write up!

@antoniobusrod
Copy link

Hi @lpinca , is it possible to keep using different WS servers for different paths with noServer property? It seems it doesn't (I've started to do changes from 2.x in my code).

@lpinca
Copy link
Member Author

lpinca commented May 23, 2017

@antoniobusrod see the above example. Can't you do that?

@antoniobusrod
Copy link

hi @lpinca , I'm preparing a PR, so let's discuss there

@sugir93
Copy link

sugir93 commented Oct 6, 2017

Thanks for the solution (Y)

@mayeaux
Copy link

mayeaux commented Dec 23, 2017

Can this be written somewhere in the docs? Lost a good chunk of time trying to figure out why one server worked but adding another blew up with an Invalid frame header error

@yiakwy
Copy link

yiakwy commented May 20, 2018

I am using the pattern. Can anybody explain to me what's meaning of websocket.emit("connection")? can it work together with websocket.on("connection", ws)? It seems that ws should be a handler not a socket for eventemiter

If I replace ws with a function named as `connection, I will get an error

[ws.handleUpgrade] emit connection event.
events.js:167
      throw er; // Unhandled 'error' event
      ^

Error [ERR_STREAM_WRITE_AFTER_END]: write after end
    at writeAfterEnd (_stream_writable.js:243:12)
    at Receiver.Writable.write (_stream_writable.js:292:5)

@yiakwy
Copy link

yiakwy commented May 20, 2018

Here is my code:

function connection (socket, req) {
	/*https://github.com/websockets/ws/pull/1099*/
	if ('upgradeReq' in socket) {
		req = socket.upgradeReq
	}

	const loc = url.parse(req.url, true).pathname
	console.log(`[server:connection] on connection from ${loc}`)
	if (!originIsAllowed(loc)) {
		/*
		 * @desc handle unwanted request
		 */ 
		console.warn(`Visit from ${loc} is not allowed.`)
	}

	req.__handled = false 
	let res = new http.ServerResponse(req)
	req._websocket = ws
	// @see express-ws-router tricks
	res.writeHead = function(status) {
		if (status > 200) {
		debug("rejected %s, status %s", loc, status)
		socket.close()
		req.__handled = true
		}
	}

	console.log(`[server:connection] begin to handle using express router system ...`)
	app.handle(req, res, function() {
		if (!req.__handled){
		debug("websocket connection %s not handled", loc)
		socket.close()
		}
	})
	/*
	socket.on('message', function incoming(message){
		console.log('received: %s', message)
	})

	socket.send("connection built successfully!")
	*/

	/*
	 * @desc register websocket routers
	 */
	
}
ws.on("connection", connection)

/*
 * @desc This techniques allow multiple websockets used, @see 
 */ 
function onUpgrade(req, HTTPSocket, head) {
 	const loc = url.parse(req.url).pathname

	switch(loc) {
		case "/sim_world":
			// WebSocketServer handle it 
			// console.log(`Receive websocket connection request ${stringify(req, null, 2)} from ${loc}`)
			console.log(`[server:onUpgrade] receive websocket connection request ${req} from ${loc}`)
			ws.handleUpgrade(req, HTTPSocket, head, (socket) => {
				// debug("ws.handleUpgrade %s, %s", socket, req) 
				console.log("[ws.handleUpgrade] emit connection event.")
				ws.emit('connection', socket)
			})
			break
		default:
			console.log(`Not recognized websocket request ${loc}`)
			HTTPSocket.destroy()
	}

}

Here is my output

[server:onUpgrade] receive websocket connection request [object Object] from /sim_world
[ws.handleUpgrade] emit connection event.
events.js:167
      throw er; // Unhandled 'error' event
      ^

Error [ERR_STREAM_WRITE_AFTER_END]: write after end
    at writeAfterEnd (_stream_writable.js:243:12)
    at Receiver.Writable.write (_stream_writable.js:292:5)
    at Socket.socketOnData (/Users/lei.wang1/Github/RandomMapGenerator/backend/node_modules/ws/lib/websocket.js:788:35)
    at Socket.emit (events.js:187:15)
    at Socket.EventEmitter.emit (domain.js:442:20)
    at addChunk (_stream_readable.js:280:12)
    at readableAddChunk (_stream_readable.js:265:11)
    at Socket.Readable.push (_stream_readable.js:220:10)
    at TCP.onread (net.js:638:20)
Emitted 'error' event at:
    at Receiver.receiverOnError (/Users/lei.wang1/Github/RandomMapGenerator/backend/node_modules/ws/lib/websocket.js:698:13)
    at Receiver.emit (events.js:182:13)
    at Receiver.EventEmitter.emit (domain.js:442:20)
    at writeAfterEnd (_stream_writable.js:245:10)
    at Receiver.Writable.write (_stream_writable.js:292:5)
    [... lines matching original stack trace ...]
    at TCP.onread (net.js:638:20)

@sugir93
Copy link

sugir93 commented May 21, 2018

Check my working code.

var server = http.createServer().listen(300);
var wss_event = new WebSocket.Server({
noServer: true
});
server.on('upgrade', function (request, socket, head) {
var pathname = url.parse(request.url).pathname;

if (pathname === '/sim_world') {
    wss_event.handleUpgrade(request, socket, head, function (ws) {
        wss_event.emit('connection', ws);
    });
} else {
    socket.destroy();
}

});

@ifree92
Copy link

ifree92 commented Aug 26, 2019

Yeah, thanks! That's working for me likewise.
In addition, if someone uses websockets and socket.io servers in one instance (yes, I'm also surprised, but I haven't other way), do not close the socket in upgrade function:

else {
    socket.destroy();
  }

facebook-github-bot pushed a commit to facebook/metro that referenced this pull request Aug 4, 2021
Summary:
## Context

Closes #674.

There is a security vulnerability with the current version of ws, that requires it to be upgraded to 5.2.3. I upgraded it to 7.5.1 while I was it.

## In this diff

A few of the API changes that needed to be fixed:
- `upgradeReq` was removed from the `WebSocket` object, the fix being to take the URL from the `request` param instead.

- `onError` now correctly passes an `ErrorEvent` instead of an `Error` object

- [can't attach more than one websocket per http server](websockets/ws#885 (comment)): this is the big one. On metro we use multiple websocket servers to do different things: HMR, JS debugger and generally talking back and forth with the dev server. The trick is to basically have an on `upgrade` handler that does the manual routing between the various servers. This is a breaking change for anybody who was using the Metro api to then attach a web socket server.

**The new recommended way to attach a websocket server if need be** is to pass a `websocketEndpoints` argument to the Metro runtime options in `Metro.runServer` that looks like `{ [path: string] : <an instance of WebSocketServer created with the "noServer: true" option> }`.

Reviewed By: motiz88

Differential Revision: D30012392

fbshipit-source-id: e69503f1a4da2ee417e7dcc9d42680e4dc0415d8
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

Successfully merging this pull request may close these issues.

8 participants