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

Fix retries for requests with a body #231

Merged
merged 3 commits into from Feb 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions index.js
Expand Up @@ -389,10 +389,10 @@ class Ky {
}

if (this._options.timeout === false) {
return globals.fetch(this.request);
return globals.fetch(this.request.clone());
}

return timeout(globals.fetch(this.request), this._options.timeout, this.abortController);
return timeout(globals.fetch(this.request.clone()), this._options.timeout, this.abortController);
}

/* istanbul ignore next */
Expand Down
48 changes: 43 additions & 5 deletions test/browser.js
Expand Up @@ -4,6 +4,8 @@ import {serial as test} from 'ava';
import createTestServer from 'create-test-server';
import withPage from './helpers/with-page';

const pBody = util.promisify(body);

test('prefixUrl option', withPage, async (t, page) => {
const server = await createTestServer();
server.get('/', (request, response) => {
Expand Down Expand Up @@ -73,22 +75,24 @@ test('throws TimeoutError even though it does not support AbortController', with
response.end();
});

server.get('/endless', () => {});
server.get('/slow', (request, response) => {
setTimeout(() => {
response.end('ok');
}, 1000);
});

await page.goto(server.url);
await page.addScriptTag({path: './test/helpers/disable-abort-controller.js'});
await page.addScriptTag({path: './umd.js'});

// TODO: make set a timeout for this evaluation so we don't have to wait 30s
const error = await page.evaluate(url => {
window.ky = window.ky.default;

const request = window.ky(`${url}/endless`, {timeout: 500}).text();
const request = window.ky(`${url}/slow`, {timeout: 500}).text();
return request.catch(error_ => error_.toString());
}, server.url);
t.is(error, 'TimeoutError: Request timed out');

// A note from @szmarczak: await server.close() hangs on my machine
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was hanging for me, too. I think because the server's response stream was never being closed. That behavior doesn't seem necessary for this test, so I simply made the Ky timeout less than the server timeout and it works as expected.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! ☺️

await server.close();
});

Expand Down Expand Up @@ -180,12 +184,13 @@ test('throws if does not support ReadableStream', withPage, async (t, page) => {
});

test('FormData with searchParams', withPage, async (t, page) => {
t.plan(2);

const server = await createTestServer();
server.get('/', (request, response) => {
response.end();
});
server.post('/', async (request, response) => {
const pBody = util.promisify(body);
const requestBody = await pBody(request);

t.regex(requestBody, /bubblegum pie/);
Expand All @@ -211,6 +216,8 @@ test('FormData with searchParams', withPage, async (t, page) => {
});

test('headers are preserved when input is a Request and there are searchParams in the options', withPage, async (t, page) => {
t.plan(2);

const server = await createTestServer();
server.get('/', (request, response) => {
response.end();
Expand All @@ -236,3 +243,34 @@ test('headers are preserved when input is a Request and there are searchParams i

await server.close();
});

test('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) => {
requestCount++;
await pBody(request);
response.sendStatus(502);
});

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

const error = await page.evaluate(url => {
window.ky = window.ky.default;
const request = window.ky(url + '/test', {
body: 'foo',
method: 'PUT',
retry: 2
}).text();
return request.catch(error_ => error_.toString());
}, server.url);
t.is(error, 'HTTPError: Bad Gateway');
t.is(requestCount, 2);
Copy link
Collaborator Author

@sholladay sholladay Feb 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reasons that I do not understand, the requestCount assertion was failing and I was only able to solve it by changing the status code from 408 to something else. When the status is 408, the server receives roughly double the number of requests I am expecting (i.e. roughly 2x whatever retry is). When I set the retry option to 1, then requestCount was 2 and the assertion passed. When I set retry to 8, then requestCount was 14 and the assertion failed (note: not quite double).

So, of course it seems like a bug in Ky. And sure enough, when I debugged it with await puppeteer.launch({ devtools: true }); in test/helpers/with-page.js, I was able to see these requests in the Network tab and the timings looked like they were respecting our backoff algorithm, so these requests are likely being triggered within Ky.

The thing is, we have no special handling of 408, nor 502 in Ky (other than being retriable), so I don't see how this could be fixed by changing from 408 to 502. I also wasn't able to easily reproduce this behavior manually in Chrome using endpoints from https://httpstat.us/. To me, it's a mystery.

I chose to just change the status code in commit 2808b0f.


await server.close();
});