nodejs / undici

An HTTP/1.1 client, written from scratch for Node.js
https://nodejs.github.io/undici
MIT License
6.23k stars 541 forks source link

bug: created readable for response without body #3588

Open PandaWorker opened 1 month ago

PandaWorker commented 1 month ago

I noticed that a reader is being created for the response body, which is not there and it is explicitly specified in content-length: 0

I also think that we could reject the allocation, the response chunk parse.body via http, and after receiving the completion of the headers, it will switch to FixedLengthReader()/ChunkedReader(), I think that parsing all chunks leads to a slowdown, since you need to make an allocation for each chunk, specify the size to wasm, and then get a steamed chunk through the callbacks

Do we need extra operations on the chunk, or can we just install the reader (ReadableStream?) from the socket

import { once } from 'node:events';
import net from 'node:net';
import { Headers, fetch, request } from 'undici';

async function createNetServer(port: number, response: Uint8Array) {
    const server = net
        .createServer(async socket => {
            for await (const chunk of socket) {
                console.log(`request:`);
                console.log(chunk.toString());

                console.log(`response:`);
                console.log(new TextDecoder().decode(response));
                socket.write(response);
                socket.end();
            }
        })
        .listen(port);
    await once(server, 'listening');
    return server;
}

const CRLF = '\r\n';

function buildResponse(
    status: number,
    headers: Headers,
    statusText: string = ''
) {
    var raw = `HTTP/1.1 ${status} ${statusText}` + CRLF;

    for (const [name, value] of headers) {
        raw += `${name}: ${value}` + CRLF;
    }

    raw += CRLF;
    return new TextEncoder().encode(raw);
}

const headers = new Headers([['content-length', '0']]);
const headers2 = new Headers([['transfer-encoding', 'chunked']]);

const response204 = buildResponse(204, headers);
const response200 = buildResponse(200, headers);

async function main() {

    const server200 = await createNetServer(3001, response200);
    const server204 = await createNetServer(3000, response204);

    // resp 204 without resp.body
    const resp = await fetch('http://localhost:3000', {
        method: 'GET',
        headers: new Headers([['host', 'myhost.local']])
    });
    console.log(resp);

    // resp 200 (content-length: 0) without resp.body ?
    const resp2 = await fetch('http://localhost:3001', {
        method: 'GET',
        headers: new Headers([['host', 'myhost.local']])
    });
    console.log(resp2);
    const respText = await resp2.text();

    console.log({respText});

    // resp 200 (content-length: 0) without resp.body ?
    const resp3 = await request('http://localhost:3001', {
        method: 'GET',
        headers: new Headers([['host', 'myhost.local']])
    });
    console.log(resp3);
    const respText3 = await resp3.body.text();

    console.log({respText3});

    // close servers
    server204.close();
    server200.close();
}

main()
PandaWorker commented 1 month ago

headers: { 'content-length': '0' }

request:
GET / HTTP/1.1
host: myhost.local
connection: keep-alive

response:
HTTP/1.1 200 
content-length: 0

{
  statusCode: 200,
  headers: { 'content-length': '0' },
  trailers: {},
  opaque: null,
  body: BodyReadable {
    _events: {
      close: undefined,
      error: undefined,
      data: undefined,
      end: undefined,
      readable: undefined
    },
    _readableState: ReadableState {
      highWaterMark: 65536,
      buffer: [],
      bufferIndex: 0,
      length: 0,
      pipes: [],
      awaitDrainWriters: null,
      [Symbol(kState)]: 1053452
    },
    _read: [Function: bound resume],
    _maxListeners: undefined,
    [Symbol(shapeMode)]: true,
    [Symbol(kCapture)]: false,
    [Symbol(kAbort)]: [Function: abort],
    [Symbol(kConsume)]: null,
    [Symbol(kBody)]: null,
    [Symbol(kContentType)]: '',
    [Symbol(kContentLength)]: 0,
    [Symbol(kReading)]: false
  },
  context: undefined
}
{ respText3: '' }
ronag commented 1 month ago

Sorry, I don't understand. What is the problem?

PandaWorker commented 1 month ago

Why create a Readable object for the response if there is none. You can just specify null. I don't understand why to perform these unnecessary operations, for answers without a body

Ethan-Arrowood commented 1 month ago

This is interesting idea. Can you try this with a browser? as well as with the Node.js IncomingMessage instance? I'd love to see how other existing clients behave.

May be worth exploring the HTTP spec for content-length: 0 too to see if it specifies that the body should not exist or be empty.

KhafraDev commented 1 month ago

https://github.com/nodejs/undici/issues/82