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

Adding Websocket support to core #19308

Open
MylesBorins opened this issue Mar 13, 2018 · 146 comments · Fixed by #49830
Open

Adding Websocket support to core #19308

MylesBorins opened this issue Mar 13, 2018 · 146 comments · Fixed by #49830
Labels
feature request Issues that request new features to be added to Node.js. never-stale Mark issue so that it is never considered stale

Comments

@MylesBorins
Copy link
Member

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

@MylesBorins MylesBorins added the feature request Issues that request new features to be added to Node.js. label Mar 13, 2018
@eugeneo
Copy link
Contributor

eugeneo commented Mar 13, 2018

Re: Inspector.

  1. Inspector WS implementation is not complete, e.g. there is no support for binary frames.
  2. Inspector would still need a C++ implementation that can run on a separate thread. Both JS execution and main libuv loop are suspended when the application hits a breakpoint.

@devsnek
Copy link
Member

devsnek commented Mar 13, 2018

i'm not wholly against this but i would like to factor in how much stuff we put into all our release binaries. if we can come up with more creative ways for shipping stuff like this i'm totally a +1 (#19307)

@mscdex
Copy link
Contributor

mscdex commented Mar 13, 2018

I am still -1 on this.

@devsnek
Copy link
Member

devsnek commented Mar 13, 2018

@mscdex can you be explicit in your reasoning?

@mscdex
Copy link
Contributor

mscdex commented Mar 13, 2018

@devsnek for the same reasons I gave in the linked issue.

@lpinca
Copy link
Member

lpinca commented Mar 13, 2018

As discussed in #1010 and as a maintainer of ws I'm +1 on adding WebSocket to core.

@MylesBorins
Copy link
Member Author

@mscdex in #1010

-1 WebSockets is still something better suited for userland IMHO. Sure it's something that many people do use, but there are also many other widely used standardized protocols/APIs that could be argued as "core" to the "web" that are not currently included in node/io.js core. For example: multipart parsing/generation, Server-sent events, HTTP/2, ICMP, SSH/SFTP, FTP, SOCKS, VNC/RFB, SMTP/IMAP/POP3, SOAP, Web Workers (as an API), XHR/XHR2 (as an API), etc.

Since this original post we've added http2. While you listed a bunch of protocols not all of them are supported natively by the browser.

https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API

https://caniuse.com/#search=Websockets

@mscdex
Copy link
Contributor

mscdex commented Mar 13, 2018

@MylesBorins I don't think node should aim to become a (DOM-less) browser. Anyway, my "vote" and reasoning still stands.

@targos
Copy link
Member

targos commented Mar 13, 2018

It's more about communicating with browsers, not becoming one.

@bnoordhuis
Copy link
Member

http2 is an argument against being too eager to absorb protocols into core. Neither its API nor its implementation are all that great; it would have benefited from iterating outside core for a while.

I guess you could construe that as an argument in favor of websockets: third-party modules have existed for years and their APIs and implementations have pretty much crystallized by now.

@brianleroux
Copy link

This is a great idea. While today Node is already an indispensable tool for web developers it does not enjoy the same full seat at the table of web browser tech advancement despite being held completely captive by it. Few agree on the controversial edicts like Promise and esmodules but we can definitely all agree these transition moments could have been handled better with Node being an a fully active participant instead of recipient of these challenges.

Node has a big opportunity to become a full fledged user agent (web browser) and first class support for web features will be a part of that. +1!

@jasnell
Copy link
Member

jasnell commented Mar 13, 2018

http2 is an argument against being too eager to absorb protocols into core. Neither its API nor its implementation are all that great;

PRs welcome.

Re: websockets

I'm still -1 for the time being. This is something that has been done quite well by userland and there are still unanswered open standards questions about http2+ws that require more thought and experimentation.

@watson
Copy link
Member

watson commented Mar 13, 2018

When http2 was added, I think one of the considerations were that we could do a lot of low level stuff in C++ land that was hard or even impossible(?) to do efficiently in user-land. Is there a similar reason for wanting to bring WS into core, or is it just to have more features?

@MylesBorins
Copy link
Member Author

@jasnell can you point me towards the open standards discussion regarding h2 + ws?

@jasnell
Copy link
Member

jasnell commented Mar 13, 2018

The ietf httpbis working group is working on this .... https://tools.ietf.org/html/draft-ietf-httpbis-h2-websockets-00

It's still super early in the process tho there is some early implementation happening.

@kof
Copy link
Contributor

kof commented Mar 13, 2018

Ideally there should be a clear general framework on how to decide whether something belongs to the core or not. I would consider points like

  • is it hard to be done right?
  • is it widely used?
  • does it have a clear spec?
  • is it likely to become obsolete in the near future?
  • does it need optimizations from the core?
  • will the entire community benefit from it being part of the core?

@addaleax
Copy link
Member

is it hard to be done right?

@kof I fully agree with all your points except this one – there is no reason to believe that code in Node core is better-written or better-maintained than userland code in general.

I think a better question in its place would be “Is it hard to do effeciently without native addons?”.

@kof
Copy link
Contributor

kof commented Mar 13, 2018

@addaleax The idea behind that point is: if it is in userland, it is likely there will be multiple competing implementations, which will result in attention not being fully focused on a single code base and as a result lower quality, just because less people are working on it or using it. I have no data to back it up.

Userland is good when a feature needs experimentation.

@lpinca
Copy link
Member

lpinca commented Mar 13, 2018

@kof

is it hard to be done right?

No.

is it widely used?

Yes it is.

does it have a clear spec?

Yes.

is it likely to become obsolete in the near future?

No.

does it need optimizations from the core?

Not necessarily but optimizations in core may help. In ws we have two optional binary addons. One for the masking/unmasking of frames (bufferutil) and one for the UTF-8 validation (utf-8-validate).

will the entire community benefit from it being part of the core?

Yes I think the community will benefit from WebSocket in core.

There are a lot of userland implementations. The most popular are ws, faye-websocket, and websocket. Combining the the best parts of each one of them to create a single module in core would be great imho.

@mcollina
Copy link
Member

mcollina commented Mar 13, 2018

+1, mainly because @lpinca has been doing a great job in maintaining ws the last few years.

@MylesBorins
Copy link
Member Author

pinging @jcoglan and @theturtle32

@watson
Copy link
Member

watson commented Mar 13, 2018

So far I haven't seen any concrete comments on why implementing WS in core would be better than having it in user-land. I'm sure @lpinca have done a great job maintaining ws, but that hardly qualifies as a reason to bring it into core right?

I just think there need to be concrete examples on how core can provide a better WS implementation than user land 😃

@devsnek
Copy link
Member

devsnek commented Mar 13, 2018

i don't think there's anyone saying we would do a better job. i think the argument is better integration and since we can ship it with a native backing users will get a perf boost without having to install anything.

@watson
Copy link
Member

watson commented Mar 13, 2018

@devsnek Could you provide an example of how the integration would be better if WS was in core?

Regarding native backing then n-api and pre-compiled binaries should remove the requirement of having users compile stuff (if I understood your argument correctly).

@TimothyGu
Copy link
Member

It seems the main argument for adding ws to Core is to reduce binary dependencies. Since both native dependencies are buffer-based, I think they may be good candidates for adoption of WebAssembly. @lpinca has that been seen as a possibility?

@MylesBorins
Copy link
Member Author

For an API, I'd like to propose the WhatWG WebSocket API, as much as we can implement the standard (I know there will be some inconsistencies around events).

If we run into any serious problems with the API design we can attempt to work with the whatwg to update the standard. It seems like @TimothyGu has participated in this document along with @domenic.

re: http/2 + ws, if we get an implementation going we can participate in trying to solve this problem!

will the entire community benefit from it being part of the core?

I think implementing standards in core, especially ones implemented in the webplatform, can help the ecosystem focus on creating better abstractions on top of a protocol like WS, and avoid having to implement the base protocol themselves. If the various ecosystem modules share a kernel it allows there to be collaboration across projects with a shared interest, and potentially brings more people to help maintain core itself.

@lpinca
Copy link
Member

lpinca commented Mar 14, 2018

@watson userland implementations work and I'm a big supporter of small core but HTTP(s) and many other modules are in core because they are so popular that it made sense to create a core module for them. The same is valid for WebSocket in my opinion. Quoting @domenic from nodejs/NG#10 (comment)

I think the conclusion of this line of thought leads us to a process wherein we say "yes, core is interested in supporting feature X." Then, someone---maybe an io.js collaborator, or maybe not!---goes off and builds a prototype of feature X as a user-land module. Along the way, they might need to ask for more low-level APIs from core, and those will get rolled in. But over time, this external implementation of feature X matures, and io.js collaborators start commenting on it, and the ecosystem starts using it, until we decide: yes, we should roll this in to core, and start shipping with it.

Is core interested in supporting WebSocket? That's an open question. I think it should.

@TimothyGu we didn't explore that possibility but that's definitely a good idea. We already have prebuilt and n-api based binaries for native addons. Removing binary dependencies is not the reason why I would like to see WebSocket in core though. The question is, why is HTTP and HTTP2 in core and WebSocket isn't? They all are in the same league and they all can live in userland if wanted. On top of that a good part of the WebSocket implementation is already in core as the upgrade mechanism is already available and supported in core.

@MylesBorins If we ever decide to experiment with WebSocket in core, API need to be discussed. I think we want to support piping data around so a websocket should be implemented as a Duplex stream like faye-websocket does. I guess we also want to support fragmented messages and permessage-deflate. In this case we should augment the standard send() to add the ability to specify if a message is the last one in a fragmented message and to specify if a message should be compressed or not.

@M3kH
Copy link

M3kH commented Mar 14, 2018

I can't establish a vote for this, although would be a nice to have.

I found that WebSocket implementation isn't that solid neither in the Browser.

Is high cpu consuming and so far I couldn't see any implementation in a big scale (which I would have guess they would be using it); eg: Facebook, Twitter and Gmail, they rather relay on polling.

I guess is mainly related on the cost created on the Server side due being really hard to scale it (eg. Load balancing). Not sure Node wants to make his capacity in supporting it.

@lpinca
Copy link
Member

lpinca commented Dec 3, 2022

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
Copy link
Member

KhafraDev commented Dec 3, 2022

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

This comment was marked as off-topic.

@github-actions github-actions bot added the stale label Jun 2, 2023
@Bessonov

This comment was marked as off-topic.

@github-actions github-actions bot removed the stale label Jun 3, 2023
@Bessonov

This comment was marked as off-topic.

@github-actions github-actions bot removed the stale label Jun 5, 2023
@Mesteery Mesteery closed this as completed Jun 5, 2023
@Mesteery Mesteery reopened this Jun 5, 2023
@tniessen tniessen added the never-stale Mark issue so that it is never considered stale label Jun 5, 2023
@prettydiff
Copy link
Contributor

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
Copy link
Contributor

Proposal for a standards compliant client and server in less than 900 lines of code: #49478

@galvez
Copy link

galvez commented Oct 12, 2023

Should this issue really be closed?

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

@MylesBorins MylesBorins reopened this Oct 12, 2023
@bnoordhuis
Copy link
Member

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
Copy link

aral commented Oct 13, 2023 via email

@piranna
Copy link
Contributor

piranna commented Oct 13, 2023

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
Copy link
Member

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

@piranna
Copy link
Contributor

piranna commented Oct 13, 2023

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
Copy link

shellscape commented Oct 13, 2023

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
Copy link

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
Copy link
Member

mcollina commented Feb 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. never-stale Mark issue so that it is never considered stale
Projects
Status: Pending Triage
Development

Successfully merging a pull request may close this issue.