oven-sh / bun

Incredibly fast JavaScript runtime, bundler, test runner, and package manager – all in one
https://bun.sh
Other
74.3k stars 2.78k forks source link

socket.write does not work with `node:http` #9882

Open jamesdiacono opened 7 months ago

jamesdiacono commented 7 months ago

What version of Bun is running?

1.1.0+5903a6141

What platform is your computer?

Darwin 23.3.0 arm64 arm

What steps can reproduce the bug?

Run this in Bun:

import http from "node:http";
const server = http.createServer();
server.on("upgrade", function (req, socket) {
    console.log("writing...");
    socket.write("x", function () {
        console.log("...written");
    });
});
server.listen(8888, "127.0.0.1");

Run this in a browser:

new WebSocket("ws://127.0.0.1:8888");

What is the expected behavior?

The "x" byte to be transferred over the TCP connection, and the following lines written to stdout:

writing...
...written

What do you see instead?

Only

writing...

and no data is transferred over the TCP connection, according to Wireshark. Both Node.js and Deno behave as expected.

Additional information

No response

Jarred-Sumner commented 7 months ago

socket in node:http is a no-op currently. We don't expose it because the HTTP server manages the socket

We expect to reimlpement how node:http works internally but won't get around to it for another couple months most likely

jamesdiacono commented 7 months ago

That explains it, thanks.

keverw commented 4 months ago

Just ran into this issue, glad I found this issue since felt like I was going crazy!

Was trying to destory the connection early if unauth'ed, etc.

However ws upgrade works for some reason to ws.send but you couldn't close from the server side.

webSocketServer.handleUpgrade(request, socket, head, (ws) => {

Trying to port some old server code I wrote, but also just wanted more control/integration into my custom framework. But after going in circles isolated it to this:

var http = require('http'); // using require so I can also run it in Node to test

const server = http.createServer((req, res) => {
  res.writeHead(200, { 'Content-Type': 'text/plain' });
  res.end('Hello, World!');
});

server.on('upgrade', (req, socket, head) => {
  // Check if it's a WebSocket upgrade request
  if (
    req.headers['upgrade'] &&
    req.headers['upgrade'].toLowerCase() === 'websocket'
  ) {
    // Respond with a 426 Upgrade Required error
    req.socket.write(
      'HTTP/1.1 426 Upgrade Required\r\n' +
        'Upgrade: WebSocket\r\n' +
        'Connection: Upgrade\r\n' +
        'Content-Type: text/plain\r\n' +
        '\r\n' +
        'This server does not support WebSocket connections.',
    );
    req.socket.destroy();
  } else {
    // For non-WebSocket upgrade requests, you can handle them differently or just close the connection
    req.socket.destroy();
  }
});

const PORT = 3000;
server.listen(PORT, () => {
  console.log(`Server running on http://localhost:${PORT}`);
});

idk if that code is useful but might help for a future test case.

On Node:

kevinwhitman@kevins-mbp ~ % websocat ws://localhost:3000
websocat: WebSocketError: WebSocketError: Received unexpected status code (426 Upgrade Required)
websocat: error running

On Bun, it just hangs forever unless I stop the server.

Looks like my new plan is to to just use Bun for package manager, bundling, cli tools and testing.... but target Node and run within Node for now until this and #7716 fixed, then switch back. Kinda frustrated but at least Bun supports targeting Node, so don't have to redo anything it seems with my codebase so far.

jamesgpearce commented 3 months ago

I just hit this trying to intercept the upgrade process and wasted a day before I discovered Bun mocks in its own implementation of ws! 🤯

The key revelation in that replacement module is this:

https://github.com/oven-sh/bun/blob/c552cb40d1b919754acf30388a4b887a65e6df7e/src/js/thirdparty/ws.js#L1167-L1169

So if you were planning to socket.write or socket.end back to the client like the real ws module does... bad luck.

Like me, you might instead need to hack something like this:

const kBunInternals = Symbol.for('::bunternal::');
const [_, response] = socket[kBunInternals];
response.writeHead(...);
response.end();

BTW, a common use case for this is rerouting websocket connections so all the clients in a given room are on the same server. The Fly.io proxy, for example will replay requests to the correct server instance if you respond to the upgrade with a magic fly-replay header, and this is the only place to do it.

So yes, it would be great if the Bun mock for the socket behavior included the rest of the implementation, and didn't instead just hang silently 🤣