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

WebSocket #932

Closed
ronag opened this issue Aug 6, 2021 · 21 comments · Fixed by #1795
Closed

WebSocket #932

ronag opened this issue Aug 6, 2021 · 21 comments · Fixed by #1795
Labels
enhancement New feature or request fetch
Milestone

Comments

@ronag
Copy link
Member

ronag commented Aug 6, 2021

https://fetch.spec.whatwg.org/#websocket-protocol

@ronag ronag added enhancement New feature or request fetch labels Aug 6, 2021
@ronag ronag mentioned this issue Aug 6, 2021
39 tasks
@mcollina
Copy link
Member

mcollina commented Aug 6, 2021

cc @lpinca

@szmarczak
Copy link
Member

@lpinca
Copy link
Member

lpinca commented Aug 7, 2021

@szmarczak I know it is a demo but that example basically skips the opening handshake. There is no validation and no subprotocol/extension negotiation.

@ronag ronag added this to the stable fetch milestone Aug 12, 2021
@WenheLI
Copy link

WenheLI commented Oct 25, 2021

Interested in this one. Would try to implement if it is availabe!

@joshxyzhimself
Copy link
Contributor

Are there any advantages in building websockets here on top of undici compared to what exists in https://github.com/websockets/ws? (maybe performance?)

@mcollina
Copy link
Member

I'm totally ok in using ws for this. It's a great module by @lpinca and I hope we will emebed it in core one day.

@jodevsa
Copy link
Contributor

jodevsa commented Nov 26, 2022

@mcollina this would be really a great opportunity for me to learn more about WS. Can I start working on this?

@mcollina
Copy link
Member

I don't think we should do another implementation, I think the ws module is great, we might just pull it in as a dep.

cc @lpinca @KhafraDev

@KhafraDev
Copy link
Member

KhafraDev commented Nov 26, 2022

ws is great but I think the implementation is too different from the spec WebSocket. There's also outstanding issues where it uses a custom EventTarget (websockets/ws#1818). Then there's a bunch of webidl issues I could spot with a cursory glance (Websocket extends EventEmitter, underscore indexed properties) and the fact that WPTs aren't being run.

Undici (or maybe the nodejs org) should fork or add ws as a git submodule where we could make breaking changes from the ws api & run WPTs, to ensure cross compatibility.


Basically I see this as another spec fetch vs. node-fetch issue. Both might share a similar API but are different enough that there isn't a cross-compatibility guarantee.

@lpinca
Copy link
Member

lpinca commented Nov 26, 2022

Following the https://websockets.spec.whatwg.org/ spec in every single detail is a non-goal for ws. It is limiting and there is no real advantage in my opinion.

I think jsdom runs the websockets web-platform-tests. They use ws under the hood with some minor adjustments.

@mcollina
Copy link
Member

I think we should have our own implementation then. Maybe reusing ws or some parts of it under the hood.

@jodevsa
Copy link
Contributor

jodevsa commented Nov 26, 2022

@mcollina i will give it a try and formulate a plan before implementing so that we don't waste time

@jodevsa
Copy link
Contributor

jodevsa commented Nov 26, 2022

I think we should have our own implementation then. Maybe reusing ws or some parts of it under the hood.

I'm still not quite sure about how we want to implement it. Do we wanna use ws as a dependency inside undici or do we want to implement WS from scratch and maybe get some ideas from ws?

@mcollina
Copy link
Member

Wdyt @KhafraDev?

@KhafraDev
Copy link
Member

KhafraDev commented Nov 26, 2022

Using ws:

  • uses http(s) under the hood
  • no undici Dispatcher
  • less security risk

Creating our own:

  • faster websockets (probably)
  • can use undici dispatchers (do websockets work with proxies?)
  • more potential security risks

If possible, having our own implementation that borrows from ws would be the ideal solution. We would need to implement the entirety of the WebSocket class in either case, for the reasons mentioned above ("Following the ... spec ... is a non-goal for ws").

@lpinca
Copy link
Member

lpinca commented Nov 27, 2022

  • faster websockets (probably)

The WebSocket protocol has nothing to do with HTTP apart from the initial handshake. Once the initial connection is established the work is done against a {net,tls}.Socket. I'm sure a new implementation would be faster but it won't be faster just because you can reuse something in undici. You have to write a faster parser and message writer. Also, the parser and message writer are not the bottlenecks in ws. The bottleneck is net.Socket. Of course, ws adds some overhead over a plain net.Socket, that is inevitable. A new implementation can try to reduce that overhead, for example by corking writes, but I think it will not be much faster.

  • can use undici dispatchers (do websockets work with proxies?)

Yes, they do, but not all HTTP proxies can proxy WebSocket.


ws was not designed to be used in fetch. This part of the WHATWG spec https://websockets.spec.whatwg.org/#websocket-protocol alters RFC 6455 and refers to browser related things (CSP, HSTS) that do not make sense in a server side context. It should be implemented from scratch.

Other ws deviations from the WHATWG spec that I can think of are:

  • The WebSocket class inherits from EventEmitter instead of EventTarget. Inheriting from EventTarget negatively affects performance and is limiting.
  • No direct Blob supports. There is no Blob support for Writable.prototype.write() in Node.js.
  • Custom additions and events.

@KhafraDev
Copy link
Member

KhafraDev commented Nov 29, 2022

I took a shot at it and got close (I think) to a working initial handshake. If anyone wants a place to start, it might be worth checking my branch out https://github.com/KhafraDev/undici/tree/ws/lib/websocket. It doesn't currently work btw (the onUpgrade callback is never called).

@jimmywarting
Copy link
Contributor

jimmywarting commented Dec 1, 2022

👍 for rolling our own that is more spec compatible with wpt tests and webidl stuff
would like things like EventTarget instead of EventEmitter and blob support.

I would really much like it if it where possible to get a blob from the file system, send it with socket.send(blob) and for it to be no data passing via the main thread. Data would instead just be directly read from the disc to the socket with c and uses a faster system call (Like Bun.js blobs) same goes for other fs method like writing/copying/moving blobs could just use fast paths instead of passing data via the js stack

@KhafraDev
Copy link
Member

KhafraDev commented Dec 1, 2022

I took another stab at the initial handshake and got it working! The mistake was really dumb on my part (sha1 is a supported hash, not SHA-1 and some error swallowing).

import { WebSocket } from './lib/websocket/websocket.js'
import { getGlobalDispatcher } from './index.js'

new WebSocket('wss://echo.websocket.events/')

Anyone is free to work/contribute to my branch - help would be appreciated(!!). I'm making some progress, but there is still a lot left to do.


Fetch is now integrated with the websocket spec & the initial handshake has been rewritten to follow the websocket spec where applicable.

@jodevsa
Copy link
Contributor

jodevsa commented Dec 2, 2022

Hi @KhafraDev,

great start! I'm really interested in working with you on this. I hope I can make some progress today; I'll try to leave some handovers in the PR comments

@jodevsa
Copy link
Contributor

jodevsa commented Dec 4, 2022

Hi @KhafraDev,

I was able to implement sending TEXT data and do the masking logic; we are currently able to send and receive text messages through WS:

const { WebSocket } = require('./lib/websocket/websocket.js')

const ws = new WebSocket('wss://echo.websocket.events/')

ws.addEventListener('message', (event) => {
  console.log('received:', event.data) // "echo.websocket.events sponsored by Lob.com"
})

ws.addEventListener('open', () => {
  let i = 0
  setInterval(() => {
    const text = `hello. This is message number #${i}`
    console.log('sending:', text)
    ws.send(text)
    i++
  }, 1000)
})

Screenshot 2022-12-04 at 01 16 21

please have a look at my PR: KhafraDev#30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fetch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants