nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.8k stars 29.7k forks source link

Adding Websocket support to core #19308

Closed MylesBorins closed 4 months ago

MylesBorins commented 6 years ago

The original thread where adding this was discussed #1010 was closed with a decision by the iojs TC to rather implement lower level buffer methods, but that was abandoned.

There is an open EPS to add the feature, but we have since abandoned the process.

Some of the people who originally were -1 changed their opinions in #1010 more recently. In fact, we already ship a partial implementation of ws in the inspector.

I think it might be worth us revisiting adding WS to core.

/cc @eugeneo @rauchg

lpinca commented 5 years ago

FWIW I've recently added in ws a utility function to wrap a ws WebSocket in a Duplex stream with full backpressure support.

karimhm commented 4 years ago

+1 on this. So no one would forget it.

aral commented 3 years ago

A new year, a new +1.

(As someone whose rollup bundle is currently breaking due to use of optional binaries in – the most excellent and wonderful – ws. goes to figure out a hack)

galvez commented 3 years ago

@aral FWIW I'm on the Fastify stack now and this is pretty solid: https://github.com/fastify/fastify-websocket

reyadussalahin commented 3 years ago

And another +1. No one should forget.

64J0 commented 3 years ago

I'm not sure if I can vote on this issue because I'm only a Node.js user but if I can there's another +1.

buzzy commented 3 years ago

+100 ;)

Assassin-1234 commented 3 years ago

+69 :DD

shellscape commented 3 years ago

BT

CodeBradley commented 3 years ago

+1

alanxp commented 3 years ago

+1

arxpoetica commented 2 years ago

Interesting development in the world of deno: https://deno.com/blog/deploy-streams#websockets

PodaruDragos commented 2 years ago

Hello guys Is the any progress regarding a consensus on this particular feature ?

51L3N7-X commented 2 years ago

it will be uesful

3rd-Eden commented 2 years ago

It still blows my mind that we still don't have this, a cli arg parser does get added, but a webstandard does not 🙃

arxpoetica commented 1 year ago

@KhafraDev Am I understanding this correctly that this is going to go into core Node?

KhafraDev commented 1 year ago

That's the plan; undici currently implements node's fetch (including Headers, Response, Request, and FormData). My implementation is very far from complete though - only the initial handshake and receiving unfragmented messages is possible currently.

Plus, we need tests and most likely many months of bug fixing. Somewhere around there we'd likely have to work on performance, as my current goals are to get a functioning prototype. Afterwards, the tsc would need to decide whether or not to add it to core, and only then do we need to wait for a release cycle or two for it to be stable.

To sum it up: I am working on it, but I wouldn't expect it in core anytime soon.

arxpoetica commented 1 year ago

To sum it up: I am working on it, but I wouldn't expect it in core anytime soon.

Amazing! Looking forward to that day!

cjihrig commented 1 year ago

Is undici the best place for a WebSockets implementation? What about the server side implementation (because undici advertises itself as an HTTP 1/.1 client)?

(FWIW, I am in favor of WebSockets in core)

kapouer commented 1 year ago

Good news, what was wrong with forking uwebsockets ? Not american enough ?

KhafraDev commented 1 year ago

Is undici the best place for a WebSockets implementation?

The websocket spec makes use of the fetch spec internals for making the initial handshake. It's still possible to do so with node:http/s (as ws does), but it's much easier implementing it in undici. Undici also implements a subset of the webidl spec, which makes implementing everything related to specs 10x easier (and generally more correct).

What about the server side implementation?

I don't think undici is the correct place to implement a WebSocketServer, that would probably be better in core.

Good news, what was wrong with forking uwebsockets ?

Nothing, I'm just not that good at C++ lol. If someone else decided to try implementing websockets, I'd be all for it - but this issue is closing in on half a decade of being open so I wouldn't count on it.

lpinca commented 1 year ago

I don't think undici is the correct place to implement a WebSocketServer, that would probably be better in core.

If there is a WebSocket client in Node.js core, it would be natural to have a WebSocket server in core. People will obviously ask for it. It would also be natural for the server and client to use the same WebSocket class. This is why I think that a 100% compliant WHATWG spec implementation is not suitable for a server-side context.

To add to https://github.com/nodejs/undici/issues/932#issuecomment-1328091686 and https://github.com/nodejs/undici/issues/932#issuecomment-1328189667:

You would have a WebIDL-compliant implementation but it would be useless (at least for me).

cjihrig commented 1 year ago

@lpinca what would you advocate for?

KhafraDev commented 1 year ago

it would be natural to have a WebSocket server in core

I'm not saying that we shouldn't have a WebSocket server in core, I'm just saying that undici (as an http 1.1 client) isn't the correct place to implement it. I haven't looked into it though, that's just my opinion.

No backpressure handling.

I don't think this is the case (at least in my current implementation). If it is, we should be able to add it, as it uses tls sockets under the hood. Is this something that the spec forbids? For instance fetch isn't 100% compliant with the spec; we've diverged in the past for the best user experience (removed header filtering, "manual" redirect mode where you can read the headers & body, etc).

You would have a WebIDL-compliant implementation but it would be useless (at least for me).

The idea isn't to supersede ws (both can exist at the same time), the goal is more so to create an implementation that works cross-environment.


I would also like to point out that the spec is often lackluster for power users, and that's completely fine IMO. The ecosystem is moving towards cross-compatibility - Deno recently (maybe not so recently) released a WebSocket client; bun has a WebSocket client; browsers have a WebSocket client. Node is currently the only platform that doesn't, and for the most part people seem fine with the limitations you've pointed out.

lpinca commented 1 year ago

@lpinca what would you advocate for?

I don't have a direct answer. As written in this thread a long time ago I'm fine with WebSocket support in core but the browser API was designed a long time ago and is limiting without additions.

I don't think this is the case (at least in my current implementation).

websocket.send() as defined by WHATWG spec gives the user no way to know when to stop writing. Polling websocket.bufferedAmount is impractical.

The WebSocketStream API is still limiting but makes more sense. Anyway I'm not sure if it is still being worked on.

Deno recently (maybe not so recently) released a WebSocket client; bun has a WebSocket client; browsers have a WebSocket client. Node is currently the only platform that doesn't, and for the most part people seem fine with the limitations you've pointed out.

Those are all features that will be requested sooner or later. Telling people that we are not going to implement them because they are not compatible with the WHATWG spec seems bad to me.

I think that apart from browsers they also have a WebSocket server. If the server uses the same WebSocket class used by the client, then it is important to also think about server requirements when implementing it.

KhafraDev commented 1 year ago

Telling people that we are not going to implement them because they are not compatible with the WHATWG spec seems bad to me.

This is generally how web proposals go though. We've turned down a bunch of requests to add things to fetch that weren't spec compliant; focusing on correctness over being feature-filled. I'd also like to add that the more features we add that other platforms don't have, the harder it becomes to use (ie. tutorials may no longer work; behavior might change between platforms, etc.). I don't think we need to tell users to pound sand either; I'd be fine recommending ws for users who need features not covered by the spec.

websocket.send() as defined by WHATWG spec gives the user no way to know when to stop writing.

I feel like this could be solved via diagnostics channel support? We could have a channel that emits a message whenever there's nothing else to send.

If the server uses the same WebSocket class used by the client, then it is important to also think about server requirements when implementing it.

Agreed 👍. My main goal is to get a working WebSocket class first, and then consider WebSocketStream and a WebSocketServer (although I still stand by my earlier opinion).

WebSocket and WebSocketStream don't need to be exclusive of one another, it's totally possible to support both. For example, Deno supports both.

lpinca commented 1 year ago

This is generally how web proposals go though. We've turned down a bunch of requests to add things to fetch that weren't spec compliant; focusing on correctness over being feature-filled. I'd also like to add that the more features we add that other platforms don't have, the harder it becomes to use (ie. tutorials may no longer work; behavior might change between platforms, etc.). I don't think we need to tell users to pound sand either; I'd be fine recommending ws for users who need features not covered by the spec.

Node.js already had http.request() when fetch was added. Turning down feature requests is easy in this case. You tell users to use another core module. Telling people to use a userland module for basic needs related to something newly added in core does not seem right to me. I mean why adding it in core then?

I feel like this could be solved via diagnostics channel support? We could have a channel that emits a message whenever there's nothing else to send.

Perhaps. The 'message' events have the same issue though, there is no way to stop them. If incoming data is piped to a slower destination, there should be a mechanism that allows to pause the websocket.

mcollina commented 1 year ago

I mean why adding it in core then?

Compatibility with Web standards. The ws module is great and if we want better APIs that are production ready we should either recommend or embed it.

KhafraDev commented 1 year ago

ws shouldn't be embedded for the reasons mentioned in https://github.com/nodejs/undici/issues/932

It doesn't integrate nicely with core features (EventTarget, Blob, and fetch). IMO it's better off as an npm package.

mcollina commented 1 year ago

You misunderstood me. I'm saying that it's not possible to implement a WebSocket class that focuses on production stability and at the same time follow the spec. Because of its lack of flow control mechanisms, it's even farther away from being built for servers then fetch() (even that is a stretch).

The ws module is production-capable implementation of the websocket protocol. I would be totally ok to have it exposed as http.ws. It's battle tested and built for Node.js. It can also stay as its own module on npm.

Any spec-compliant WebSocket implementation would need to come with caveats like:

Implemented only for compatibility reason, do not use in any heavy traffic production server.

lpinca commented 1 year ago

To be honest I struggle to understand why anyone would open a WebSocket connection via fetch in an environment where HSTS, CSP, CORS, etc. have no meaning. Wouldn't it be simpler/easier to use the old WebSocket constructor?

The only reason I can think of is reusing code already written for the browser (the cross-compatibility argument mentioned above, which is honorable) but I'm not sure if it is good enough in this case ¯_(ツ)_/¯. Making trade-offs is hard.

KhafraDev commented 1 year ago

Wouldn't it be simpler/easier to use the old WebSocket constructor?

Both fetch support and the WebSocket class are being added in my PR, so it's entirely up to the user. The websocket spec already uses fetch internals for making the initial handshake so integration wasn't hard.

Currently you can open a websocket connection in 2 ways with my pr:

const ws = new WebSocket('wss://url', [optional protocols])

await fetch('wss://url')
github-actions[bot] commented 1 year ago

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

Bessonov commented 1 year ago

non-automated comment

Bessonov commented 1 year ago

@bnoordhuis any comment on why you added the label again?

prettydiff commented 1 year ago

This thread is old and contains much history, so please forgive me if I miss anything.

WebSocket is defined as RFC 6455. Everything else is API and implementation specific. To achieve complete inter-agent conformance in the most primitive way possible you only need to adhere to RFC 6455 which only includes Node's net (insecure sockets/servers) and tls libraries.

Most Node WebSocket implementations execute as an HTTP upgrade. That is how the browser implements this, but that is not necessary. RFC 6455 provides no mention of HTTP except for specific plain text in a one time connection header. This means a WebSocket is merely a TLS or TCP raw socket with:

  1. 1 time connection headers at both client and server response (plain text).
  2. Binary RFC 6455 frame headers of 2-14 bytes.
  3. A mask algorithm: RFC 6455, 5.3
  4. Optional message segmentation

Required components, tasks essential to achieve RFC 6455 specific criteria:

  1. connect - This opens a client to a server with a plain text payload conforming to the RFC 6455 connection message. This features a:
    1. .once("ready", handler)
    2. create a RFC 6455 security nonce
    3. write plain text connection headers
    4. .on("data", handler)
    5. callback, which may include custom extensions
  2. receiver - This receives messages off the socket, which includes unmasking, frame segmentation, and executing control frames. The receiver must be applied to all sockets irrespective of role (client or server). Segmentation includes:
    • TLS max packet size
    • header separation, FireFox tends to send frame headers separated from frame bodies
    • Node concatenation, high frequency message transfer from Node may result in messages concatenated into single TCP packets
    • RFC 6455 conformant message segmentation
  3. listener - This is just a TCP server with extended functionality for receiving/responding to incoming client connections and their plain text header payloads. It can include custom logic, such as authentication mechanisms, to arbitrary drop incoming sockets. Security/authentication exceeds the scope of RFC 6455.
  4. sender - This provides any requested message segmentation and forms the frame header before writing to the socket. Messages and their respective segments must be written in the order of message provided and segmentation. An exception to that order are control frames, which may be injected at any time irrespective of sequencing.

The above list is all I have written in my own implementation to make this work with browsers and other Node instances. I need to review RFC 6455 and see what features I ignored as extraneous for my own needs, for example the connection security nonce is required by RFC 6455 (and implemented by the browsers) but is not necessary in practice.

Also RFC 6455 requires that messages to be masked if sent from a client, but not if sent from a server. Browsers strongly adhere to this. I have found this otherwise to be entirely unnecessary as it only exists to isolate message traffic from interference and outside the browser role (client/server identity) becomes irrelevant after socket establishment. Nonetheless, this must exist by default to achieve RFC 6455 conformance and should be disabled via option.

Nice to have list of optional features, things unique to WebSocket implementation but not required to achieve RFC 6455 transmission:

  1. identity - Socket identity is critically important for application maintenance to prevent false assumptions when dealing with multiple open sockets and cannot be left to transmission details (ip/url) alone.
  2. type identifier - An arbitrary string to identify the purpose or utility of a given socket.
  3. authentication - WebSockets have life beyond the web/browser. In most other environments automated means to determine when to drop incoming connections is very important.
  4. role distinction - It is helpful for troubleshooting to determine if a given socket at a given location is connected as a client or server by simply examining a property on the socket object.
  5. logging - message logging is nice for security and maintenance reasons. A separated logging feature beyond an inspector allows for piping message details to files or streaming to databases and so forth.
  6. perf - Performance testing is nice to determine maximum send/receive frequency for determining required resources. It takes far greater effort to receive messages than to send them because message segmentation is trivial but message consolidation requires examination of the frame header and examination of the message body(bodies).
  7. no client masking - Simply provide an option to turn off masking from client-side sockets.
  8. broadcasts - A convenience method to send a given message to a plurality of sockets.

Everything else should be deferred to user land dependencies or other Node core libraries. There are some gotchas in writing the code that require playing with socket establishment and managing multiple sockets simultaneously.

prettydiff commented 1 year ago

Proposal for a standards compliant client and server in less than 900 lines of code: https://github.com/nodejs/node/pull/49478

galvez commented 1 year ago

Should this issue really be closed?

We still do not have a WebSocket server in Node.js core.

bnoordhuis commented 1 year ago

What's the way forward here? Unlike client WS, there's no WS server standard to follow.

Before anyone suggests copying commercial cloud vendors' proprietary APIs: no. Just no.

aral commented 1 year ago

Nearly everyone uses ws at the moment, no? Pave the cow paths? :)

Aral

On Fri, Oct 13, 2023, at 10:25 AM, Ben Noordhuis wrote:

What's the way forward here? Unlike client WS, there's no WS server standard to follow.

Before anyone suggests copying commercial cloud vendors' proprietary APIs: no. Just no.

— Reply to this email directly, view it on GitHub https://github.com/nodejs/node/issues/19308#issuecomment-1761202968, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKGRQATENGA7DXYKPIFBDX7ECJLANCNFSM4EU7UOJQ. You are receiving this because you were mentioned.Message ID: @.***>

piranna commented 1 year ago

Nearly everyone uses ws at the moment, no? Pave the cow paths? :)

There are others, but yes, ws is one of the most populars, not only because it's fast (but uws is faster), but also because ws server side API is VERY similar to client side (and browsers) one, so as reference API I find it to be the correct one to mimick.

mcollina commented 1 year ago

Ultimately I don't see a reason to have a server implementation, ws works extremely well.

piranna commented 1 year ago

Ultimately I don't see a reason to have a server implementation, ws works extremely well.

Performance, maybe? I like ws, and usually I forgot to use the binary add-ons, but maybe have ws integrated in core and with the binary add-ons and some other optimizations that could be done being in core, could be an improvement...

shellscape commented 1 year ago

eh, I like having ws independent. We got the client, and that's good. I wouldn't want to see a Node core implementation of ws suffer the same nuetered fate as parseArgs did as compared to existing, established packages.

yoursunny commented 9 months ago

I have an app currently using WebSocket client from ws. I investigated the possibility of switching to Node.js builtin WebSocket client, and found a feature missing: I cannot specify the AddressFamily of the connection. In ws this was possible, because their WebSocket constructor can pass along options to http.request, which accepts a family option. My app performs monitoring the remote WebSocket servers and I need the AddressFamily option so that I can check the target over both IPv4 and IPv6.

mcollina commented 9 months ago

That is expected. ws does not follow the specification, but it's a node.js specific implementation.

The ask is to support WebSocket as you find it in browsers.

We might add additional options in the future, but the goal is spec compatibility on first instance.

RedYetiDev commented 4 months ago

Shouldn't this been solved by https://github.com/nodejs/node/pull/49830?

mcollina commented 4 months ago

Closed by https://github.com/nodejs/node/pull/49830