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

bug: created readable for response without body #3588

Open
PandaWorker opened this issue Sep 12, 2024 · 5 comments
Open

bug: created readable for response without body #3588

PandaWorker opened this issue Sep 12, 2024 · 5 comments
Labels
question [Use a Discussion instead]

Comments

@PandaWorker
Copy link

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 PandaWorker added the bug Something isn't working label Sep 12, 2024
@PandaWorker
Copy link
Author

headers: { 'content-length': '0' }
[Symbol(kContentLength)]: 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
Copy link
Member

ronag commented Sep 12, 2024

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

@PandaWorker
Copy link
Author

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

@ronag ronag added question [Use a Discussion instead] and removed bug Something isn't working labels Sep 12, 2024
@Ethan-Arrowood
Copy link
Collaborator

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

#82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question [Use a Discussion instead]
Projects
None yet
Development

No branches or pull requests

4 participants