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

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

Closed
tnfAngel opened this issue Mar 3, 2024 · 10 comments
Closed

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

tnfAngel opened this issue Mar 3, 2024 · 10 comments

Comments

@tnfAngel
Copy link

tnfAngel commented Mar 3, 2024

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

vibl commented Mar 15, 2024

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

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 [email protected] (as mentioned above by @vibl) and [email protected]

balloman added a commit to balloman/status-bot that referenced this issue May 14, 2024
fixed an issue that would cause ky to not parse json correctly. Fixed
according to this issue:
sindresorhus/ky#563 (comment)
@ershisan99
Copy link

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 [email protected] (as mentioned above by @vibl) and [email protected]

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

@sholladay
Copy link
Collaborator

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:

ky/source/core/Ky.ts

Lines 81 to 107 in 585ebcb

for (const [type, mimeType] of Object.entries(responseTypes) as ObjectEntries<typeof responseTypes>) {
result[type] = async () => {
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
ky.request.headers.set('accept', ky.request.headers.get('accept') || mimeType);
const awaitedResult = await result;
const response = awaitedResult.clone();
if (type === 'json') {
if (response.status === 204) {
return '';
}
const arrayBuffer = await response.clone().arrayBuffer();
const responseSize = arrayBuffer.byteLength;
if (responseSize === 0) {
return '';
}
if (options.parseJson) {
return options.parseJson(await response.text());
}
}
return response[type]();
};
}

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

tnfAngel commented Jun 30, 2024

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

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/[email protected]';

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

tnfAngel commented Jul 1, 2024

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/[email protected]';

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

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

tnfAngel commented Jul 1, 2024

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 openapi-ts/openapi-typescript#1486

@tnfAngel tnfAngel changed the title ky response is an empty string sometimes await ky.get('url').json() or .text() is an empty string sometimes Jul 1, 2024
@sholladay
Copy link
Collaborator

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:
oven-sh/bun#6468

@sholladay sholladay closed this as not planned Won't fix, can't repro, duplicate, stale Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants