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

node:http incorrectly send Transfer-Encoding: chunked even if no body is written #20063

Closed
osddeitf opened this issue Aug 4, 2023 · 2 comments · Fixed by #20127
Closed

node:http incorrectly send Transfer-Encoding: chunked even if no body is written #20063

osddeitf opened this issue Aug 4, 2023 · 2 comments · Fixed by #20127
Labels
bug Something isn't working correctly node compat

Comments

@osddeitf
Copy link
Contributor

osddeitf commented Aug 4, 2023

Original issue: #20033.

After debugging for a while, I found out that the HTTP PUT request was sent with Transfer-Encoding: chunked header, although the body is empty.

Which is different from Nodejs behavior:
The header is only added by the first calling request.write(chunk):

If no Content-Length is set, data will automatically be encoded in HTTP Chunked transfer encoding, so that server knows when the data ends. The Transfer-Encoding: chunked header is added.

These are the headers Nodejs would send (PUT with no body):

Headers {
  connection: "close",
  "content-length": "0",
  "content-type": "text/plain",
  host: "127.0.0.1:8001"
}

And these are the headers by Deno:

Headers {
  accept: "*/*",
  "accept-encoding": "gzip, br",
  "content-type": "text/plain",
  host: "127.0.0.1:8001",
  "transfer-encoding": "chunked",
  "user-agent": "Deno/1.36.0"
}

I tried digging into Deno source code to investigate the issue, and found that currently all POST, PATCH and PUT requests will be considered to have body:

  • ext/node/polyfills/http.ts
    this._req = core.ops.op_node_http_request(
      this.method,
      url,
      headers,
      client.rid,
      this.method === "POST" || this.method === "PATCH" ||
        this.method === "PUT",
    )
  • ext/node/ops/http.rs (these lines are useless as a result)
    // POST and PUT requests should always have a 0 length content-length,
    // if there is no body. https://fetch.spec.whatwg.org/#http-network-or-cache-fetch
    if matches!(method, Method::POST | Method::PUT) {
      request = request.header(CONTENT_LENGTH, HeaderValue::from(0));
    }

I don't know how to change the code to fix the issue yet. But I think of 2 ways:

  • Add hasBody and hasContentTypeHeader flags then check them in request.write function to add Transfer-Encoding: chunked (but I don't know whether the reqwest crate supports adding header after started).
  • Only start sending reqwest after the first call to request.write.

I'd appreciate any help to implement this.

@bartlomieju bartlomieju added bug Something isn't working correctly node compat labels Aug 5, 2023
@bartlomieju bartlomieju self-assigned this Aug 5, 2023
@bartlomieju
Copy link
Member

CC @crowlKats can you take a look and provide some pointers for @osddeitf to fix it?

@bartlomieju bartlomieju removed their assignment Aug 6, 2023
@osddeitf
Copy link
Contributor Author

osddeitf commented Aug 7, 2023

The current behavior in nodejs is waiting for the first body chunk, or socket close(), until then the headers would be sent:

  • lib/_http_outgoing.js
  this._header = header + '\r\n';
  this._headerSent = false;

  // Wait until the first body chunk, or close(), is sent to flush,
  // UNLESS we're sending Expect: 100-continue.
  if (state.expect) this._send('');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly node compat
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants