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

Response.text() may be broken #10467

Closed
lowlighter opened this issue May 2, 2021 · 2 comments
Closed

Response.text() may be broken #10467

lowlighter opened this issue May 2, 2021 · 2 comments
Assignees
Labels
ext/fetch related to the ext/fetch invalid what appeared to be an issue with Deno wasn't

Comments

@lowlighter
Copy link
Contributor

lowlighter commented May 2, 2021

When decoding a Response body with .text() method, output may not be what is actually expected.

Below is a repro:

const file = await Deno.open(new URL("test.json", import.meta.url), { read: true })
const original = await Deno.readTextFile("test.json")
let copy = ""
const body = new ReadableStream<Uint8Array>({
  start: async (controller) => {
    for await (const chunk of Deno.iter(file)) {
      copy += new TextDecoder().decode(chunk)
      controller.enqueue(chunk)
    }
    file.close();
    controller.close();
  }
});
const fetched = await new Response(body, {status: 200}).text()

//Tests
console.log(original === copy) //Should be true
console.log(original === fetched) //Should be true
console.log(original.length, copy.length, fetched.length) //Should all be equals
console.log(original.substring(0, 20))
console.log("======================")
console.log(copy.substring(0, 20))
console.log("======================")
console.log(fetched.substring(0, 20))

Outputs the following:

PS > deno run -A .\test.ts
true
false
72218 72218 72218
{
  "pins": {

======================
{
  "pins": {

======================
719,
        -1212,

Original, copy and fetched strings should be the same but for some reason fetched content doesn't start at correct place.

Since it works fine by pushing directly to a string variable with TextDecoder, I assume it's come either from controller.enqueue or the Response.body handling

PS > deno --version
deno 1.9.2 (release, x86_64-pc-windows-msvc)
v8 9.1.269.5
typescript 4.2.2

Link to tested JSON file
(Original issue lucacasonato/deno_local_file_fetch#8 (comment))


Edit: Here's a workaround in case anyone need it:

const body = await fetch(new URL("test.json", import.meta.url)).then(res => res.body)
const reader = body?.getReader() as ReadableStreamDefaultReader<Uint8Array>
let text = ""
while (true) {
  const {done, value} = await reader.read()
  text += new TextDecoder().decode(value)
  if (done)
    break
}

I think the bug comes somewhere from how body is consumed or packaged in extensions/fetch/22_body.js because I get the bug in .text(), .json() and .blob(). Looking at source code I figured out that .body does neither of them so you can just consume the stream yourself to make it work 🙂

@kitsonk kitsonk added needs investigation requires further investigation before determining if it is an issue or not ext/fetch related to the ext/fetch labels May 2, 2021
@lucacasonato
Copy link
Member

After breaking my head over this for about an hour:

Deno.iter
Iterator uses an internal buffer of fixed size for efficiency; it returns a view on that buffer on each iteration. It is therefore caller's responsibility to copy contents of the buffer if needed; otherwise the next iteration will overwrite contents of previously returned chunk.

Thus, the issue is that you do this:

controller.enqueue(chunk);

instead of this:

controller.enqueue(chunk.slice(0));

Thus, not a Deno bug.

@lucacasonato lucacasonato added invalid what appeared to be an issue with Deno wasn't and removed needs investigation requires further investigation before determining if it is an issue or not labels May 29, 2021
@lowlighter
Copy link
Contributor Author

Nice thanks a lot for debugging it and sorry for this 😅

I submitted a PR to lucacasonato/deno_local_file_fetch#10 to apply your patch since originally I opened this because it was the implementation of local files fetching in deno deploy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext/fetch related to the ext/fetch invalid what appeared to be an issue with Deno wasn't
Projects
None yet
Development

No branches or pull requests

3 participants