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

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

Open
jamesdiacono opened this issue Apr 3, 2024 · 4 comments
Open

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

jamesdiacono opened this issue Apr 3, 2024 · 4 comments
Labels
bug Something isn't working node:http node.js Compatibility with Node.js APIs

Comments

@jamesdiacono
Copy link

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

@jamesdiacono jamesdiacono added the bug Something isn't working label Apr 3, 2024
@Jarred-Sumner
Copy link
Collaborator

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

That explains it, thanks.

@Electroid Electroid added the node.js Compatibility with Node.js APIs label Apr 9, 2024
@Electroid Electroid changed the title socket.write does nothing inside "upgrade" listener socket.write does not work with node:http Apr 9, 2024
@keverw
Copy link

keverw commented Jul 14, 2024

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

jamesgpearce commented Aug 3, 2024

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:

bun/src/js/thirdparty/ws.js

Lines 1167 to 1169 in c552cb4

handleUpgrade(req, socket, head, cb) {
// socket is actually fake so we use internal http_res
const [_, response] = socket[kBunInternals];

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 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working node:http node.js Compatibility with Node.js APIs
Projects
None yet
Development

No branches or pull requests

6 participants