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 the upgradeReq property #1099

Closed
wants to merge 3 commits into from
Closed

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented May 9, 2017

This removes the upgradeReq property from the WebSocket object. The http.IncomingMessage object is instead passed as the second argument to the connection event.

In this way most of the memory retained by the http.IncomingMessage object can be GC'ed reducing memory usage by ~20%.

Developers can save only the required info or the whole http.IncomingMessage object, if they have to, when the event is emitted.

@3rd-Eden
Copy link
Member

This is a major breaking change tho.

@lpinca
Copy link
Member Author

lpinca commented May 11, 2017

This is indeed a big change. It will break a lot of existing code, but along with nodejs/node#11926 will greatly reduce memory usage.

In the worst case scenario all is needed to bring back the old behavior is this:

wss.on('connection', (ws, req) => {
  ws.upgradeReq = req;
});

Currently the opposite is needed to free the memory.

wss.on('connection', (ws) => {
  ws.upgradeReq = null;
});

Basically it will make the feature opt-in instead of opt-out.

The `http.IncomingMessage` object, instead of being attached to the
`WebSocket` object, is passes as the second argument to the
`connection` event.
@lpinca lpinca closed this May 11, 2017
@lpinca lpinca deleted the remove/upgrade-req branch May 11, 2017 07:34
@lpinca
Copy link
Member Author

lpinca commented May 11, 2017

Ops, I deleted the branch instead of force pushing 😄.
Reopened as #1104.

otofune added a commit to misskey-delta/misskey-api that referenced this pull request Jul 2, 2017
facebook-github-bot pushed a commit to facebookarchive/nuclide that referenced this pull request Oct 14, 2017
Summary:
Address the breaking changes from: https://github.com/websockets/ws/releases/tag/3.0.0
Apply the changes suggested in websockets/ws#1099
Apply the changes suggested in websockets/ws#1101

Reviewed By: hansonw

Differential Revision: D6031593

fbshipit-source-id: 6ffe8b03dd6ddab1dc26074a5d9b3baf55689d17
stefanb2 pushed a commit to stefanb2/koa-websocket that referenced this pull request Nov 8, 2017
To reduce memory usage ws replaced the socket.updateReq property with
2nd argument to the connection event [1]

[1] websockets/ws#1099
stefanb2 pushed a commit to stefanb2/koa-websocket that referenced this pull request Nov 8, 2017
To reduce memory usage ws replaced the socket.updateReq property with
2nd argument to the connection event [1]

[1] websockets/ws#1099
stefanb2 added a commit to stefanb2/koa-websocket that referenced this pull request Nov 10, 2017
To reduce memory usage ws replaced the socket.updateReq property with
2nd argument to the connection event [1]

[1] websockets/ws#1099
@projetoarduino
Copy link

Hi

Where is sec-websocket-key

@lpinca
Copy link
Member Author

lpinca commented Nov 21, 2017

@projetoarduino in the req object.

wss.on('connection', (ws, req) => {
  const key = req.headers['sec-websocket-key'];
});

@projetoarduino
Copy link

Thanks my friend.

justmoon added a commit to interledger-deprecated/five-bells-ledger that referenced this pull request Nov 23, 2017
@projetoarduino
Copy link

projetoarduino commented Nov 23, 2017

Hi, this method work, but i need one unique id per client.
I need send private messages.

Answering my own question:

The way I managed to identify the clients was this

ws._ultron.id

@Josephine7
Copy link

Hey @projetoarduino
So I am following your train of thought. I also needed unique identifiers for our customers. How did you then send that message to a particular user ?

@projetoarduino
Copy link

projetoarduino commented Dec 4, 2017

Hi use wss.clients.forEach but add one 'if' to compare and send only client you need.
You need create one object to store ws._ultron.id and associated to another data you know, one nick or Id

function senMess(get_dest, message){ wss.clients.forEach(function each(client) { console.log(client); if (client !== ws && client.readyState === WebSocket.OPEN && client.ws._ultron.id == CLIENTS[get_dest]) { client.send(message); } });

@lpinca
Copy link
Member Author

lpinca commented Dec 4, 2017

Don't use private properties as they may disappear at any time. If you need a unique id per connection generate it.

wss.on('connection', (ws) => {
  ws.id = uuid();
});

acklenx added a commit to acklenx/node-http-mitm-proxy that referenced this pull request Dec 11, 2017
an optimization in version 3.0 of the required proxy server [ websockets/ws#1114 ]drops the upgradeReq parameter to save 20% in memory.  This breaks non-http(s) requests that use upgrade to switch - like web sockets (ws and wss).  This was never a consideration for he mitm proxy (I guess since the package specifies version 3.2 and up - so it never worked).  This matters for me so we re include it per the recommendation from the package maintainers [ websockets/ws#1099 ]
@projetoarduino
Copy link

@lpinca Thanks, i tested this and work fine.

ericfranz added a commit to OSC/ondemand that referenced this pull request Nov 13, 2019
ws 3 removed upgradeReq property:

websockets/ws#1099

this is now a second argument to the connection callback i.e.

    wss.on('connection', function(ws, req){...
samirmansour pushed a commit to OSC/ondemand that referenced this pull request Feb 14, 2020
ws 3 removed upgradeReq property:

websockets/ws#1099

this is now a second argument to the connection callback i.e.

    wss.on('connection', function(ws, req){...
rjmunro added a commit to rjmunro/kurento-rtmp that referenced this pull request Apr 13, 2020
Get it in the new way, from the connection event:
websockets/ws#1099
feross added a commit to feross/simple-websocket that referenced this pull request May 17, 2020
…q support

BREAKING: Drop old Edge
- Non Chromium versions of Edge are no longer supported

BREAKING: Drop Node 10 server support
- If you use require('simple-websocket/server') then we require Node 12 minmum for ES class property syntax. If you need earlier Node support, then continue relying on the last major version.

BREAKING: Remove upgradeReq support
- upgradeReq was removed from ws in an earlier version. See websockets/ws#1099 for how to re-add it back if it's needed

Server: Pass options through to SimpleWebSocket constructor
- For example, a server can be initialized with server = new Server({ port, encoding: 'utf8' }) to make each connection stream use utf8 encoding
facebook-github-bot pushed a commit to facebook/metro that referenced this pull request Jul 26, 2021
Summary:
# Context

#674

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

# In this diff

> At a glance, we're affected by at least websockets/ws#1099 in v3.x (here) and websockets/ws@63e275e in v4.x (here). Probably a few other changes as well.

Like motiz88 mentioned in the issue, there's only 2 API changes that needed to be fixed:
- `upgradeReq` was removed from the web socket object, the fix being to take the URL from the request param instead
- `onError` now correctly passes an `ErrorEvent` instead of an `Error` object

Those are the only usages of ws in metro that i've seen

Reviewed By: GijsWeterings

Differential Revision: D29517185

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

4 participants