Encoding issue

[srenault]

Bug Description

For a single request, which as always the same response, I'm having an encoding issue that appears only from time to time.
In the response, I'm having this character � instead of special characters like é or à etc...
The encoding issue doesn't appear for all special characters in the response.
Over the time, it's not always the same characters that are impacted by the encoding issue.

Reproducible By

I used this simple gist to reproduce the issue:
https://gist.github.com/srenault/654c0179277a29b1c798461b20d5ec44

Logs & Screenshots

Environment

Node v14.16.0

Additional context

If I reset the globalDispatcher with a new Agent() before each call of request, I don't have the issue.

[koh110]

We have same issue.

I think this problem is due to the lack of consideration for multibyte characters.

This is my reproduction code.

import { createServer } from "http";
import { Client } from "undici";
import { once } from "events";
import assert from "assert";

const data = "あ".repeat(10000000);

const server = createServer((request, response) => {
  response.end(JSON.stringify({ data }));
}).listen();

await once(server, "listening");

const client = new Client(`http://localhost:${server.address().port}`);

try {
  const { body, headers, statusCode } = await client.request({
    path: "/",
    method: "GET",
  });
  console.log(`response received ${statusCode}`);
  console.log("headers", headers);
  // body.setEncoding('utf8')
  const json = await body.json();
  assert.strictEqual(json.data, data, "multi byte");
} catch (error) {
  console.error(Object.keys(error), error.code);
} finally {
  client.close();
  server.close();
}

It looks like the bug will be fixed.

diff --git a/lib/api/readable.js b/lib/api/readable.js
index 0580f42..3bb454e 100644
--- a/lib/api/readable.js
+++ b/lib/api/readable.js
@@ -219,9 +219,9 @@ function consumeEnd (consume) {

   try {
     if (type === 'text') {
-      resolve(body.join(''))
+      resolve(Buffer.concat(body))
     } else if (type === 'json') {
-      resolve(JSON.parse(body.join('')))
+      resolve(JSON.parse(Buffer.concat(body)))
     } else if (type === 'arrayBuffer') {
       const dst = new Uint8Array(length)

test code

diff --git a/test/client-request.js b/test/client-request.js
index ebb105e..3f8c128 100644
--- a/test/client-request.js
+++ b/test/client-request.js
@@ -210,6 +210,27 @@ test('request json', (t) => {
   })
 })

+test('request long multi byte json', (t) => {
+  t.plan(1)
+
+  const obj = { asd: 'あ'.repeat(100000) }
+  const server = createServer((req, res) => {
+    res.end(JSON.stringify(obj))
+  })
+  t.teardown(server.close.bind(server))
+
+  server.listen(0, async () => {
+    const client = new Client(`http://localhost:${server.address().port}`)
+    t.teardown(client.destroy.bind(client))
+
+    const { body } = await client.request({
+      path: '/',
+      method: 'GET'
+    })
+    t.strictSame(obj, await body.json())
+  })
+})

Can I send PR? @ronag @mcollina (you look like an active maintainer. Please let me know if there is an appropriate destination.)

[mcollina]

Thanks for producing this example.

I think this is a bug in Node streams and we got a reproduction.

The proposed fix is correct but significantly slower than the string-based alternative :(.

[mcollina]

Anyway, a PR will be highly welcomed.

[mcollina]

Does uncommenting body.setEncoding('utf8') fix the problem?

[koh110]

I thought it was caused by body.setEncoding('utf8') too. But unfortunately that didn't fix the problem 😢

[koh110]

I will try to create a PR for the time being. I would be grateful if you could review it.