oven-sh / bun

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

http.request and fetch on keep-alive opens always new socket, even it shouldn't #12053

Open reaby opened 3 months ago

reaby commented 3 months ago

What version of Bun is running?

1.1.15

What platform is your computer?

Linux 5.15.153.1-microsoft-standard-WSL2 x86_64 x86_64

What steps can reproduce the bug?

import { Agent } from 'http';
import { request } from 'http';
import type { Socket } from "bun";

class test {
    agent = new Agent({
        keepAlive: true,
        maxSockets: 1,
        keepAliveMsecs: 3000,
    });
    sockets: Socket[] = [];

    async call(method: string, ...params: any[]) {
        const body = "testing";
        const outData = body; // await this.compress(body);
        let headers: any = {
            "Content-Type": "text/xml",
            "Content-Encoding": "gzip",
            'Connection': 'keep-alive',
            'Content-Length': Buffer.byteLength(outData),
        };

        const res: any = await new Promise((resolve, reject) => {
            const req = request({
                hostname: 'localhost',
                port: 3000,
                path: '/',
                method: 'POST',
                headers: headers,
                agent: this.agent,
            }, (response) => {
                response.setEncoding('utf-8');
                let recvData = '';
                response.on('data', (chunk) => {
                    recvData += chunk;
                });
                response.on('end', () => {
                    resolve({ data: recvData, headers: response.headers });
                });

            });

            req.on('error', function (e) {
                console.log(e);
                reject(e);
            });

            req.on("socket", (socket: any) => {
                if (!this.sockets.includes(socket)) {
                    console.log("new socket created");
                    this.sockets.push(socket);
                    socket.on("close", function () {
                        console.log("socket has been closed");
                    });
                }
            });

            req.write(outData);
            console.log("should keepalive? ", req.shouldKeepAlive);
            req.end();
        });
        return res.data;
    }

    async test() {
        const res = await fetch("http://localhost:3000", {
            method: "POST",
            body: "test",
            headers: {
                "Content-Type": "text/xml",
                "Content-Encoding": "gzip",
                'Connection': "Keep-Alive"
            },
            keepalive: true,
            verbose: true
        });
        return await res.text();
    }
}

Bun.serve({
    fetch(req: Request): Response | Promise<Response> {
        if (req.method == "POST") {
            return new Response("Testing...");
        } else {
            return new Response("Hello World!");
        }
    },
    port: 3000,
});

const t = new test();

setInterval(async () => {
    const data: any = await t.call("test", []);
    console.log(data);
    // const data2: any = await t.test();
    // console.log(data2);
}, 2000);

What is the expected behavior?

should keepalive? true new socket created Testing... should keepalive? true Testing... should keepalive? true Testing...

What do you see instead?

should keepalive? true new socket created Testing... should keepalive? true new socket created Testing... should keepalive? true new socket created Testing...

Additional information

I tried everything, but bun http agent and http stack opens always new socket, and doesn't close the previous... even set to maxsockets = 1 and keepalive....

reaby commented 3 months ago

double checked the code, on nodejs it just works as intented.

reaby commented 3 months ago

also i found out that Agent is never called from the request, i changed the default agent to custom one (https://github.com/telefonica/node-http-pooling-agent) in hopes to get this working... it just uses the default, thus faulty, http agent.

Chris92de commented 3 months ago

Same issue exists on 1.1.16 and 1.1.17. The Connection: keep-alive header is being completely ignored. This needs fixing.

pmbanugo commented 2 months ago

Any plans to add this to the roadmap? @Jarred-Sumner or do a triage from one of the core contributors?

reaby commented 2 months ago

btw, it's fine, i switched to use nodejs as this been isseue so long time for my project. You can close the issue as it seems nobody else needs any of this