sindresorhus / ky

🌳 Tiny & elegant JavaScript HTTP client based on the browser Fetch API
MIT License
11.83k stars 341 forks source link

await ky.get('url').json() or .text() is an empty string sometimes #563

Closed tnfAngel closed 1 week ago

tnfAngel commented 4 months ago

Bun 1.0.29 (a146856d)

await ky.get('url').json() or .text() is likely to fail on bun

Response img

Empty string when doing .json() or .text() img

vibl commented 3 months ago

I have this problem too. About 1 in 6 requests returns an empty response body in Bun (no problem in Node.js).

If I add the following line in the function_() function (line 19), I always get a response body with the cloned request, even when the main request response doesn't return a body.

        let response = await ky._fetch();
        // This prints an empty body in about 1 in 6 requests
         response.clone().blob().then(blob => blob.text()).then( str => console.log("Blob for main request:", str));
        // This always prints a body
        fetch(this.request.clone(), {}).then( res => res.blob()).then(blob => blob.text()).then( str => console.log("Blob for cloned request:", str));
Drakeoon commented 1 month ago

Got the same problem recently. In my case adding double await helped for some reason:

# ❌ before
const userDetails = await client.get(`users/${id}`).json();

# ✅ after
const userDetails = await (await client.get(`users/${id}`)).json();

Really don't know why that helps, but after this change I've never experienced any problems. I'm also using bun@1.1.3 (as mentioned above by @vibl) and ky@1.2.2

ershisan99 commented 2 weeks ago

Got the same problem recently. In my case adding double await helped for some reason:

# ❌ before
const userDetails = await client.get(`users/${id}`).json();

# ✅ after
const userDetails = await (await client.get(`users/${id}`)).json();

Really don't know why that helps, but after this change I've never experienced any problems. I'm also using bun@1.1.3 (as mentioned above by @vibl) and ky@1.2.2

That's brilliant, thank you very much! I was starting to lose my mind over this

sholladay commented 2 weeks ago

I don't know what the actual problem is, but if the "double await" example is a 100% reliable workaround, then that would seem to indicate that the problem is related to our body method shortcut function:

https://github.com/sindresorhus/ky/blob/585ebcb80545d784b31033e5a70326a0eb202468/source/core/Ky.ts#L81-L107

Whereas a single await calls .json() on the Ky promise and thus uses our shortcut function, the double await example calls .json() on the response (just like you would with fetch()) and does not use our shortcut function. The difference should be minor, as all promise.json() does is set the Accept request header and runs a few parsing rules for convenience.

That said, the implementation does do two notable things:

  1. It calls response.clone(), which has proven to be problematic in some cases.
  2. In order to make it possible for promise.json() to set the Accept header before the request is sent even though it's a separate function call from ky(), we delay the fetch. It wouldn't surprise me if this was causing some kind of race condition.
tnfAngel commented 1 week ago

double await works fine and fully fixes the issue, but it's not nice to have to always write this workaround, this bug should be fixed as is a pain when you have an issue that you have no idea why it happens and then you realize is this

sholladay commented 1 week ago

this bug should be fixed

To do that, we need one of the following:

  1. Someone needs to provide a reproducible failing test case so that I or another maintainer can investigate the problem. Even if the problem is intermittent, the test case can just repeat itself until failure.
  2. Investigate it yourself by adding some console logs or debugger statements to the part of the code that I linked to in my last comment. Find out which of those if statements is being triggered that return an empty string and then determine how the values are different than you expected.

On my end of things, I don't use Bun and I cannot reproduce this bug in Deno or Node.

Here was my attempt to reproduce it:

import ky from 'https://unpkg.com/ky@1.3';

const url = 'https://jsonplaceholder.typicode.com/todos/1';
const maxAttempts = 300;
let attemptCount = 0;
let json;

while (attemptCount < maxAttempts) {
    attemptCount++;

    json = await ky(url).json();

    if (!json) {
        break;
    }
}

if (json) {
    console.log(`Success: receieved JSON ${attemptCount} times`);
    console.log(json);
}
else {
    console.error(`Failure: received empty JSON on attempt ${attemptCount}`);
    console.error(json);
}
tnfAngel commented 1 week ago

this bug should be fixed

To do that, we need one of the following:

1. Someone needs to provide a reproducible failing test case so that I or another maintainer can investigate the problem. Even if the problem is intermittent, the test case can just repeat itself until failure.

2. Investigate it yourself by adding some console logs or debugger statements to the part of the code that I linked to in my last comment. Find out which of those `if` statements is being triggered that return an empty string and then determine how the values are different than you expected.

On my end of things, I don't use Bun and I cannot reproduce this bug in Deno or Node.

Here was my attempt to reproduce it:

import ky from 'https://unpkg.com/ky@1.3';

const url = 'https://jsonplaceholder.typicode.com/todos/1';
const maxAttempts = 300;
let attemptCount = 0;
let json;

while (attemptCount < maxAttempts) {
    attemptCount++;

    json = await ky(url).json();

    if (!json) {
        break;
    }
}

if (json) {
    console.log(`Success: receieved JSON ${attemptCount} times`);
    console.log(json);
}
else {
    console.error(`Failure: received empty JSON on attempt ${attemptCount}`);
    console.error(json);
}

I can reproduce the bug with this exact code on bun v1.1.17 (latest), so this bug might be related to bun image Although it doesn't seem to be that common in this piece of code, in my production application when I reported it the chances of failure were ~70-80%, I don't know what factor makes this more common or not, but it was very frequent (and it still is if I do it without the workaround)

this also fails on bun for windows, and seems more frequent image image

this bug cannot be reproduced or is very unlikely to happen in node or deno image

sholladay commented 1 week ago

Yeah, this is looking like a Bun issue. It will likely reproduce with fetch instead of Ky when triggered just the right way, probably as a result of using .clone().

Plain fetch:

const url = 'https://jsonplaceholder.typicode.com/todos/1';
const maxAttempts = 300;
let attemptCount = 0;
let json;

while (attemptCount < maxAttempts) {
    attemptCount++;

    const response = await fetch(url);
    json = await response.json();

    if (!json) {
        break;
    }
}

if (json) {
    console.log(`Success: receieved JSON ${attemptCount} times`);
    console.log(json);
}
else {
    console.error(`Failure: received empty JSON on attempt ${attemptCount}`);
    console.error(json);
}

With cloned response:

const url = 'https://jsonplaceholder.typicode.com/todos/1';
const maxAttempts = 300;
let attemptCount = 0;
let json;

while (attemptCount < maxAttempts) {
    attemptCount++;

    const response = await fetch(url);
    json = await response.clone().json();

    if (!json) {
        break;
    }
}

if (json) {
    console.log(`Success: receieved JSON ${attemptCount} times`);
    console.log(json);
}
else {
    console.error(`Failure: received empty JSON on attempt ${attemptCount}`);
    console.error(json);
}

With double cloned response:

const url = 'https://jsonplaceholder.typicode.com/todos/1';
const maxAttempts = 300;
let attemptCount = 0;
let json;

while (attemptCount < maxAttempts) {
    attemptCount++;

    const response = await fetch(url);
    json = await response.clone().clone().json();

    if (!json) {
        break;
    }
}

if (json) {
    console.log(`Success: receieved JSON ${attemptCount} times`);
    console.log(json);
}
else {
    console.error(`Failure: received empty JSON on attempt ${attemptCount}`);
    console.error(json);
}

With double cloned response and decoded array buffer:

const url = 'https://jsonplaceholder.typicode.com/todos/1';
const maxAttempts = 300;
let attemptCount = 0;
let json;

while (attemptCount < maxAttempts) {
    attemptCount++;

    const response = await fetch(url);
    const buffer = await response.clone().clone().arrayBuffer();
    json = new TextDecoder().decode(buffer);

    if (json) {
        json = JSON.parse(json);
    }
    else {
        break;
    }
}

if (json) {
    console.log(`Success: receieved JSON ${attemptCount} times`);
    console.log(json);
}
else {
    console.error(`Failure: received empty JSON on attempt ${attemptCount}`);
    console.error(json);
}
tnfAngel commented 1 week ago

Yeah, this is looking like a Bun issue. It will likely reproduce with fetch instead of Ky when triggered just the right way, probably as a result of using .clone().

Plain fetch:

const url = 'https://jsonplaceholder.typicode.com/todos/1';
const maxAttempts = 300;
let attemptCount = 0;
let json;

while (attemptCount < maxAttempts) {
    attemptCount++;

    const response = await fetch(url);
    json = await response.json();

    if (!json) {
        break;
    }
}

if (json) {
    console.log(`Success: receieved JSON ${attemptCount} times`);
    console.log(json);
}
else {
    console.error(`Failure: received empty JSON on attempt ${attemptCount}`);
    console.error(json);
}

With cloned response:

const url = 'https://jsonplaceholder.typicode.com/todos/1';
const maxAttempts = 300;
let attemptCount = 0;
let json;

while (attemptCount < maxAttempts) {
    attemptCount++;

    const response = await fetch(url);
    json = await response.clone().json();

    if (!json) {
        break;
    }
}

if (json) {
    console.log(`Success: receieved JSON ${attemptCount} times`);
    console.log(json);
}
else {
    console.error(`Failure: received empty JSON on attempt ${attemptCount}`);
    console.error(json);
}

With double cloned response:

const url = 'https://jsonplaceholder.typicode.com/todos/1';
const maxAttempts = 300;
let attemptCount = 0;
let json;

while (attemptCount < maxAttempts) {
    attemptCount++;

    const response = await fetch(url);
    json = await response.clone().clone().json();

    if (!json) {
        break;
    }
}

if (json) {
    console.log(`Success: receieved JSON ${attemptCount} times`);
    console.log(json);
}
else {
    console.error(`Failure: received empty JSON on attempt ${attemptCount}`);
    console.error(json);
}

With double cloned response and decoded array buffer:

const url = 'https://jsonplaceholder.typicode.com/todos/1';
const maxAttempts = 300;
let attemptCount = 0;
let json;

while (attemptCount < maxAttempts) {
    attemptCount++;

    const response = await fetch(url);
    const buffer = await response.clone().clone().arrayBuffer();
    json = new TextDecoder().decode(buffer);

    if (json) {
        json = JSON.parse(json);
    }
    else {
        break;
    }
}

if (json) {
    console.log(`Success: receieved JSON ${attemptCount} times`);
    console.log(json);
}
else {
    console.error(`Failure: received empty JSON on attempt ${attemptCount}`);
    console.error(json);
}

I have tried all the cases and the only one that has not given an error is the first one, so it is the .clone() seems related to https://github.com/openapi-ts/openapi-typescript/issues/1486

sholladay commented 1 week ago

Yeah, this appears to be a known issue in Bun.

There is a PR to fix it which has been open for a long time: https://github.com/oven-sh/bun/pull/6468