Actually cancel the request after the timeout #27

ashinzekene · 2018-09-10T19:00:14Z

Fixes #8

sindresorhus · 2018-09-10T19:04:09Z

Awesome! Can you add a test?

sindresorhus · 2018-09-11T06:23:30Z

index.js

@@ -99,6 +105,10 @@ class Ky {
 			...otherOptions
 		};

+		if (supportsCancel) {
+			this._options.signal = abortCtrl.abort();


This would override the user's signal.

sindresorhus · 2018-09-11T06:23:45Z

index.js

@@ -1,6 +1,7 @@
 'use strict';

 const isObject = value => value !== null && typeof value === 'object';
+const supportsCancel = typeof AbortController === 'function';


supportsCancel => supportsAbortController

sindresorhus · 2018-09-11T06:24:00Z

index.js

@@ -77,6 +78,11 @@ class TimeoutError extends Error {
 	}
 }

+let abortCtrl;


abortCtrl => abortController

ashinzekene · 2018-09-11T07:53:35Z

I need help with writing the tests

sindresorhus · 2018-09-11T07:54:13Z

index.js

@@ -99,6 +105,10 @@ class Ky {
 			...otherOptions
 		};

+		if (supportsAbortController && !this._options.signal) {


We still want timeout to work, even if the user sets their own signal.

index.js

ashinzekene

Linting is failing because of this line

There is no addEventListener method on type AbortSignal

taylorjdawson · 2018-09-17T18:36:25Z

How is this PR coming along, @ashinzekene anything I can do to help?

ashinzekene · 2018-09-17T21:34:16Z

I'm waiting for a review. You can also look at helping with tests

sindresorhus · 2018-09-29T18:36:46Z

@felixfbecker You have experience with AbortController, is this the correct way to handle it?

felixfbecker

The signal should probably also be passed to delay() in the retry logic

felixfbecker · 2018-09-30T20:32:09Z

index.js

@@ -77,12 +79,21 @@ class TimeoutError extends Error {
 	}
 }

+let abortController;


A global AbortController for all requests? That seems wrong. It should be one controller per request

felixfbecker · 2018-09-30T20:32:49Z

index.js

 const delay = ms => new Promise(resolve => setTimeout(resolve, ms));

 const timeout = (promise, ms) => Promise.race([
 	promise,
 	(async () => {
 		await delay(ms);
+		if (!isAborted && supportsAbortController) {
+			abortController.abort();
+			isAborted = true;


Isn't isAborted the same as abortController.signal.aborted?

felixfbecker · 2018-09-30T20:34:22Z

index.js

@@ -99,6 +110,14 @@ class Ky {
 			...otherOptions
 		};

+		if (supportsAbortController) {
+			if (this._options.signal) {
+				this._options.signal.addEventListener('abort', () => !isAborted && abortController.abort());


abort() is idempotent so the isAborted check is not needed.

ashinzekene · 2018-10-02T16:05:49Z

I believe this fixes the issues right ? @felixfbecker

felixfbecker

Would it be possible to add a test for this? For example, if you set the timeout 0 or 1ms then wait, you should be able to assert that the Promise rejects with a TimeoutError.
Related, since this touches the AbortSignal, there should probably be a test that asserts you can still pass an AbortSignal manually (i.e. Promise is rejected with AbortError if signal is aborted).

index.js

ashinzekene · 2018-10-03T04:32:44Z

Would work on it. Thanks

…

On Tue, Oct 2, 2018 at 6:08 PM Felix Becker ***@***.***> wrote: ***@***.**** commented on this pull request. Would it be possible to add a test for this? For example, if you set the timeout 0 or 1ms then wait, you should be able to assert that the Promise rejects with a TimeoutError. Related, since this touches the AbortSignal, there should probably be a test that asserts you can still pass an AbortSignal manually (i.e. Promise is rejected with AbortError if signal is aborted). ------------------------------ In index.js <#27 (comment)>: > @@ -77,10 +78,13 @@ class TimeoutError extends Error { const delay = ms => new Promise(resolve => setTimeout(resolve, ms)); -const timeout = (promise, ms) => Promise.race([ +const timeout = (promise, ms, abortController) => Promise.race([ I don't see this parameter being passed anywhere? ***@***.***/-/blob/index.js#L81:7&tab=references — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUBOX02JhV8vcwx2EIFoYhd54gdByXZ2ks5ug52qgaJpZM4Wh_JX> .

-- Ashinze Ekene

ashinzekene · 2018-10-12T10:24:29Z

I'm finding it hard to test. I initially had to installed browser-env to be able to use AbortController on the test file, but then this still return undefined

felixfbecker · 2018-10-12T10:28:07Z

You shouldn't need a whole browser env with DOM etc for this. https://www.npmjs.com/package/abort-controller should be enough.

ianwalter · 2018-10-19T19:22:40Z

@felixfbecker I don't think node-fetch supports AbortController yet node-fetch/node-fetch#437 and I doubt jsdom does either.

It sounds like support for testing in a real browser will land in ky soon: #34

ashinzekene · 2018-10-20T12:20:27Z

Cool @ianwalter

I found out that the tests I wrote kept failing. Didn't know if the issue was from node-fetch or abort-controller polyfill.

Was about raising the issue here

szmarczak · 2018-10-25T12:30:20Z

index.js

 	promise,
 	(async () => {
 		await delay(ms);
+		if (supportsAbortController) {


if (abortController) {

index.js

szmarczak · 2018-10-25T12:43:49Z

LGTM, we need only to test it on browsers somehow...

sindresorhus · 2018-10-31T19:11:16Z

@ianwalter I've added browser testing, so you can use this now to implement the tests: f0b7502 :)

…t-controller

felixfbecker · 2018-11-08T18:51:27Z

index.js

+		if (supportsAbortController) {
+			abortController = new AbortController();
+			if (this._options.signal) {
+				this._options.signal.addEventListener('abort', () => abortController.abort());


This listener would need to be removed once the request is completed (or errored, i.e. in a finally), otherwise the listener would leak and stay around forever even though the request cannot be cancelled anymore

Believe it or not, this was never fixed before the PR landed so we ran into this memory leak in our application in 2024! 😄

sholladay · 2018-11-09T06:47:26Z

index.js

+		let abortController;
+
+		if (supportsAbortController) {
+			abortController = new AbortController();


abortController should be defined as a const, since it's never re-assigned.

sindresorhus · 2018-12-15T11:57:12Z

@ianwalter Still interested in finishing this? :)

…t-controller

ashinzekene · 2018-12-15T15:37:15Z

I've pushed fixes for this. But I need help writing tests. Can someone assist?
@felixfbecker @szmarczak @ianwalter

ianwalter · 2018-12-15T15:40:51Z

I tried writing a test for this but doesn't look like headless cypress run (electron 59) supports AbortController. Will play around with it some more.

Update: Not 100% sure but seems like we should just wait for Cypress v4 which will upgrade Electron to v3 which will use Chrome v66 which will add support for this. I read that you can't polyfill AbortController with native fetch so that's doesn't seem like an option. Other option is to use puppeteer/karmatic instead of Cypress.

felixfbecker · 2018-12-15T16:33:26Z

Yeah if you just assign AbortController on window fetch will not honor the signal. Do the test make actual HTTP requests though? What if fetch is also polyfilled (the polyfill can use xhr under the hood which is cancellable)?

ashinzekene · 2018-12-15T17:55:40Z

When I tried writing tests for it, I found out that it actually works.
I wrote this

	it('throws abort error when aborted by user', async () => {
		const abortController = new AbortController();
		const {signal} = abortController;
		const response = ky('https://jsonplaceholder.typicode.com/todos', {signal});
		expect(() => abortController.abort()).to.throw(window.AbortError);
		await response;
	});

this from cypress

The test fails but the request is actually aborted and I can't find a way to catch the error

However, if i write the test this way

	it('throws abort error when aborted by user', async () => {
		const abortController = new AbortController();
		const {signal} = abortController;
		const response = ky('https://jsonplaceholder.typicode.com/todos', {signal});
		expect(abortController.abort).to.throw(window.AbortError);
		await response;
	});

It passes but the request is not aborted

ianwalter · 2018-12-15T20:44:36Z

I would have had a couple of working tests thanks to @felixfbecker's suggestion but forgot that using fetch (even if it's a polyfill) doesn't work with cy.route stubbing (cypress-io/cypress#95) so I have to find another way to handle the request and assert it's aborted.

sholladay · 2018-12-15T22:20:50Z

@ianwalter have you seen this example of stubbing fetch in cy.visit() instead of cy.route()? https://github.com/cypress-io/cypress-example-recipes/blob/master/examples/stubbing-spying__window-fetch/cypress/integration/stub-fetch-spec.js#L17-L23

felixfbecker · 2018-12-16T04:32:52Z

Since when does window.AbortError exist?

ashinzekene · 2018-12-16T12:38:31Z

Sorry, there's actually nothing like that. I got the error from Cypress. I think its a type of DOMException

felixfbecker · 2018-12-16T14:54:04Z

error.name will be AbortError

sindresorhus · 2019-01-17T06:06:37Z

Bump :)

sindresorhus · 2019-02-04T11:06:49Z

Thank you, @ashinzekene :)

cancel request after timeout

7bf5ebe

sindresorhus changed the title ~~cancel request after timeout~~ Actually cancel the request after the timeout Sep 10, 2018

sindresorhus reviewed Sep 11, 2018

View reviewed changes

renamed variables

3034170

sindresorhus reviewed Sep 11, 2018

View reviewed changes

timer still works if user sets his own signal

9c981eb

ashinzekene commented Sep 12, 2018

View reviewed changes

index.js Outdated Show resolved Hide resolved

ashinzekene commented Sep 12, 2018

View reviewed changes

ashinzekene added 2 commits September 12, 2018 12:22

changed onabort to use event listener

d77be3f

quick fix

16a97de

felixfbecker reviewed Sep 30, 2018

View reviewed changes

ashinzekene added 2 commits October 2, 2018 16:52

removed isAborted check and global abortController

3e2807a

merge conflict with upstream

8c5e710

felixfbecker reviewed Oct 2, 2018

View reviewed changes

index.js Show resolved Hide resolved

szmarczak reviewed Oct 25, 2018

View reviewed changes

index.js Outdated

promise,

(async () => {

await delay(ms);

if (supportsAbortController) {

Copy link

Collaborator

szmarczak Oct 25, 2018

Choose a reason for hiding this comment

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

if (abortController) {

szmarczak reviewed Oct 25, 2018

View reviewed changes

index.js Outdated Show resolved Hide resolved

sindresorhus force-pushed the master branch from 383d754 to f107e7b Compare November 1, 2018 04:57

Merge branch 'master' of https://github.com/sindresorhus/ky into abor…

6cfe4bc

…t-controller

felixfbecker reviewed Nov 8, 2018

View reviewed changes

sholladay reviewed Nov 9, 2018

View reviewed changes

ashinzekene added 2 commits December 15, 2018 14:20

Merge branch 'master' of https://github.com/sindresorhus/ky into abor…

bc8e112

…t-controller

fixed issues with abort controller

f05387a

ianwalter mentioned this pull request Jan 20, 2019

Replace Cypress with AVA + Puppeteer #84

Merged

2 tasks

szmarczak and others added 4 commits February 2, 2019 18:48

Merge branch 'master' into master

bb56884

Update index.js

f6d1182

tests

5e830bf

Fix tests

2f7528e

szmarczak approved these changes Feb 2, 2019

View reviewed changes

szmarczak requested review from sindresorhus and sholladay February 2, 2019 19:04

sholladay approved these changes Feb 3, 2019

View reviewed changes

sindresorhus merged commit 8f17a13 into sindresorhus:master Feb 4, 2019

Actually cancel the request after the timeout #27

Actually cancel the request after the timeout #27

Conversation

ashinzekene commented Sep 10, 2018

sindresorhus commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashinzekene commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashinzekene left a comment • edited Loading

Choose a reason for hiding this comment

taylorjdawson commented Sep 17, 2018

ashinzekene commented Sep 17, 2018

sindresorhus commented Sep 29, 2018

felixfbecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashinzekene commented Oct 2, 2018

felixfbecker left a comment

Choose a reason for hiding this comment

ashinzekene commented Oct 3, 2018 via email

ashinzekene commented Oct 12, 2018

felixfbecker commented Oct 12, 2018

ianwalter commented Oct 19, 2018

ashinzekene commented Oct 20, 2018

Choose a reason for hiding this comment

szmarczak commented Oct 25, 2018

sindresorhus commented Oct 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Dec 15, 2018

ashinzekene commented Dec 15, 2018

ianwalter commented Dec 15, 2018 • edited Loading

felixfbecker commented Dec 15, 2018

ashinzekene commented Dec 15, 2018 • edited Loading

ianwalter commented Dec 15, 2018

sholladay commented Dec 15, 2018

felixfbecker commented Dec 16, 2018

ashinzekene commented Dec 16, 2018

felixfbecker commented Dec 16, 2018

sindresorhus commented Jan 17, 2019

sindresorhus commented Feb 4, 2019

ashinzekene left a comment •

edited

Loading

ianwalter commented Dec 15, 2018 •

edited

Loading

ashinzekene commented Dec 15, 2018 •

edited

Loading