Ky: Duplicate retries when using Chromium

[szmarczak]

Test:

test.only('retry with body', withPage, async (t, page) => {
    let requestCount = 0;

    const server = await createTestServer();
    server.get('/', (request, response) => {
        response.end('zebra');
    });

    server.put('/test', async (request, response) => {
        console.log(Date.now());
        requestCount++;
        await pBody(request);
        response.sendStatus(408);
    });

    await page.goto(server.url);
    await page.addScriptTag({path: './umd.js'});

    await new Promise(resolve => page.on('close', resolve));
    t.is(requestCount, 1);

    await server.close();
});

with-page.js:

export default async function withPage(t, run) {
    const browser = await puppeteer.launch({
        headless: false
    });
    const page = await browser.newPage();
    try {
        await run(t, page);
    } finally {
        await page.close().catch(() => {});
        await browser.close().catch(() => {});
    }
}

When you run the following code, you will notice that it will receive three requests (3 different timestamps, there is a ~5ms difference between them). Seems like Chromium (including Google Chrome) is doing retries too.

globalThis.fetch(`${location.href}test`, {
    method: 'PUT',
    body: 'a'
});

Related:

• https://www.bennadel.com/blog/3257-google-chrome-will-automatically-retry-requests-on-certain-error-responses.htm
• https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/urswDsm6Pe0
• https://medium.com/@lighthopper/connection-retry-schedule-in-chrome-browser-a9c814b7dc20

[szmarczak]

The "proper" solution would be to check all the HTTP error codes for both Google Chrome and Firefox and see when it retries.

Not only that, we need to check if the requestCount is right when getting 413: https://github.com/sindresorhus/ky/blob/78814e74efaaab65728103a13541820441b05d93/test/retry.js#L86-L148

Honestly, I'd consider this a ~Google Chrome~ Chromium (including Google Chrome) bug since fetch is called only once.

[sholladay]

Note that I was only able to reproduce this with Puppeteer driving Chromium. In my testing, normal Chrome without Puppeteer didn't display this behavior. I did not try Chromium without Puppeteer, or Puppeteer driving Chrome. We should try those next to to rule out it being a difference between Chromium and Chrome. If Chromium on its own does not display this behavior, then it likely has something to do with Puppeteer, which would be a relief.

[szmarczak]

@sholladay ~Yup, you're right. Doesn't happen for Google Chrome. I'll try to figure out why.~

[szmarczak]

It also happens for Google Chrome too. If you run fetch once, you will notice that there is only 1 requests in total. If you run fetch again, you will notice that there are 3 (!) requests in total.

[szmarczak]

~https://codereview.chromium.org/303443011~

[szmarczak]

As you can see, it's natively implemented: https://chromium.googlesource.com/chromium/src/+/refs/tags/78.0.3904.130/net/http/http_network_transaction.cc#1096

That means, we can't disable it :(

[szmarczak]

Search Chromium for "resend"
ShouldResendRequest: so it seems it does only so for keep-alive requests

[szmarczak]

I say let's stay with the current behavior, but just do

console.warn('Ky: Duplicate retries may be present as Chromium has implemented retries natively for `408 Request Timeout` errors')

when retrying on 408 on Chromium.

[sholladay]

@bthallion do you know anyone at Google who could look at this? Chromium is _implicitly_ retrying network requests, which makes it hard for fetch libraries to coordinate retries. And IIRC from my testing it's even doing so for POST requests, which are non-idempotent (!)

In my opinion, it would be pretty awesome for the browser to handle retries, so long as it's baked into the spec and can be customized via fetch options. Doing so implicitly and only in certain cases is just plain awful, though.

[sindresorhus]

Open a Chrome bug (https://bugs.chromium.org/p/chromium/issues/list) and I'll try to get it routed to the right person.

It might be worth opening an issue on Fetch about this too: https://github.com/whatwg/fetch To get the spec to clarify retry behavior. Either to explicitly allow it, make implementation defined, or to disallow it. I would first check that it's not already mentioned in the spec.

[szmarczak]

I would first check that it's not already mentioned in the spec.

Have done that already. No mention unfortunately :(

[szmarczak]

The workaround would be to set the keepalive option to false.

This is worth looking at: https://github.com/whatwg/fetch/issues/679