Add ability to cancel in-progress requests #287

alextes · 2017-03-25T11:32:10Z

Test canceling in progress request.
- Call .cancel() at the right moment.
Check if it's possible to abort during an in-progress request.
Fix the PCancelable rejection blowing up instead of hitting our catch handler.
Document cancel option

The documentation on aborting is unfortunately pretty skimpy. It appears Node destroys the socket on abort, so it sounds like we can abort during any phase of the request.

Fixes #274

sindresorhus · 2017-04-05T14:09:34Z

Yes, I think we can abort the request whenever, so I think all we have to do is make promise.cancel() call request.abort().

sindresorhus · 2017-04-05T14:10:08Z

Fix the PCancelable rejection blowing up instead of hitting our catch handler.

Where is this happening?

alextes · 2017-04-05T16:19:07Z

Hi, thanks for the feedback, happy to get it 😁.

$ ava test/http.js
body is a stream, piping
calling cancel

  1 failed

  cancel in-progress request promise
  node_modules/p-cancelable/index.js:65

   64:     this._canceled = true;
   65:     this._reject(new CancelError());
   66:   }

  Error: undefined

  Promise.cancel (node_modules/p-cancelable/index.js:65:16)
  Timeout._onTimeout (test/http.js:131:18)

  The .only() modifier is used in some tests. 11 tests were not run

sindresorhus · 2017-05-01T05:34:28Z

Would you be able to submit a failing test for p-cancelable? I never got time to dig into this. A failing test would make it much easier to investigate.

alextes · 2017-05-05T16:32:04Z

p-cancelable has been updated with a fix.

Final notes:

not sure calling abort before the request event is emitted works nicely. I used an approach that waits for the request to start then tells Node to abort.
p-cancelable returns immediately when asked to cancel a settled promise. I worry about the use-case where a user tries to cancel a settled promise but won't know the request went through. I can imagine that's problematic especially when a user doesn't care much about a response but is going for more of an RPC type deal.

alextes · 2017-05-05T16:35:20Z

Thanks, forgot to save the dep update!

sindresorhus · 2017-05-05T16:35:34Z

not sure about canceling before the request event is emitted. used an approach that waits for the request to start then tells Node to abort.

Is that really necessary? I think you can abort a request whenever.

p-cancelable immediately when asked to cancel a settled promise.

It immediately does what?

alextes · 2017-05-05T16:42:52Z

Is that really necessary? I think you can abort a request whenever.

Let me make sure and test.

It immediately does what?

Sorry. 'p-cancelable returns immediately when asked to cancel a settled promise.'

alextes · 2017-05-05T17:06:45Z

Heyo, I predicted an issue (starting to get the hang of this 😛) but forgot to make an effort to expand the test to catch the issue. I shelved it because the lovely version of the test doesn't work due to.. long story. Anyway, last two commits show the issue - not a pretty test - and apply the stash I had that solves the issue.

sindresorhus · 2017-05-05T17:29:23Z

Anyway, last two commits show the issue - not a pretty test - and apply the stash I had that solves the issue.

Ah ok. I get it now.

sindresorhus · 2017-05-05T17:29:48Z

index.js

+
+			onCancel(() => {
+				req.abort();
+			});


Test pass without this. Can you add a test to cover it?

Hmmm. Alright. That means coming up with a way to easily test the request actually gets canceled. Don't want to simply repeat what I did in the previous test. I'll come up with something clean. Give me a couple minutes.

sindresorhus · 2017-05-05T17:30:29Z

test/http.js

+	t.false(cancelRequestCompleted);
+});
+
+test.failing('cancel throws after request completed', async t => {


What is this about?

p-cancelable returns immediately when asked to cancel a settled promise. I worry about the use-case where a user tries to cancel a settled promise but won't know the request went through. I can imagine that's problematic especially when a user doesn't care much about a response but is going for more of an RPC type deal.

It works exactly like trying to reject a settled promise. When I promise is settled, it's by the name of it settled. Same with cancel. If the user use .cancel(), they should be aware they might call it too late and handle that themselves if they need. We can document this well. They can even check the promise whether it was canceled or not with promise.canceled (Which is false if you try canceling it after the promise is settled). I think p.cancel() throwing based on timing could lead to race issues and would be surprising to the user.

Please call me out if you think I'm totally wrong in this. I'm happy to discuss.

I did a little testing and a little thinking. Trying to resolve an already settled promise seems to do nothing in the case of Node too. A note in the documentation is enough.
I think you're right on this.

sindresorhus · 2017-05-06T07:19:08Z

@alextes In case you missed it, note: #287 (comment)

alextes · 2017-05-06T15:25:10Z

Not sure where to leave the documentation for this. It's a sort of indirect API. I took my best guess.

sindresorhus · 2017-05-06T18:49:25Z

test/http.js

+	p.cancel();
+
+	await t.throws(p, PCancelable.CancelError);
+	await t.notThrows(() => cancelPromise);


await t.notThrows(cancelPromise);

sindresorhus · 2017-05-06T18:50:10Z

test/http.js

+	const body = new Readable({
+		read() {}
+	});
+	body.push(String(1));


body.push('1');

sindresorhus · 2017-05-06T18:50:42Z

test/http.js

+		cancelPromise = new Promise((resolve, reject) => {
+			req.on('aborted', resolve);
+			res.on('finish', () => {
+				reject(new Error('request finished instead of aborting'));


request => Request

sindresorhus · 2017-05-06T18:51:31Z

test/http.js

@@ -1,8 +1,13 @@
+import stream from 'stream';
+import PCancelable from 'p-cancelable';


Place this after from 'ava'

Cool, so we alphabetize after complying with the import plugin rules?

No, I just prefer the order:

builtins

ava (placed here because it's the main external import)

external

local

sindresorhus · 2017-05-06T18:52:06Z

readme.md

@@ -356,6 +356,9 @@ request(`https://${config.host}/production/`, {
 });
 ```

+## Aborting the request


Can you place this above the Proxies section?

Maybe we should say Canceling the request instead?

Done :3.
I don't think we should. It sounds worse, granted, but Node consistently calls it 'abort'.

sindresorhus · 2017-05-06T18:55:01Z

readme.md

@@ -356,6 +356,9 @@ request(`https://${config.host}/production/`, {
 });
 ```

+## Aborting the request
+
+The promise returned by got features a `.cancel` function. It takes no arguments and will abort the request, destroying the underlying socket if called.


got => Got

.cancel => .cancel()

I would make it more succinct:

The promise returned by Got has a .cancel() function, which when called, aborts the request.

sindresorhus · 2017-05-06T18:55:23Z

index.js

@@ -357,6 +371,7 @@ function stdError(error, opts) {

 got.RequestError = createErrorClass('RequestError', stdError);
 got.ReadError = createErrorClass('ReadError', stdError);
+got.InvalidCancelError = createErrorClass('InvalidCancelError', stdError);


We don't need this anymore.

sindresorhus · 2017-05-06T18:57:18Z

test/http.js

+	});
+
+	await t.throws(p, PCancelable.CancelError);
+	await t.notThrows(() => cancelPromise);


await t.notThrows(cancelPromise);

I just tried this. The test fails without. Turned it back into what it was. I remember now I wrote it your suggested way first. Not sure why this demands to be lazily evaluated. My JS-foo is not strong enough (yet).

@alextes t.notThrows either accepts a promise that rejects or a function that throws. AVA is actually catching a bug in your test here as cancelPromise is null, not a Promise. Try: console.log(cancelPromise === null);

Of course. I'll fix it.

sindresorhus · 2017-05-06T18:58:27Z

Can you mention cancelation in the readme intro?

alextes · 2017-05-06T20:31:36Z

package.json

@@ -63,6 +64,7 @@
  "devDependencies": {
    "ava": "^0.18.0",
    "coveralls": "^2.11.4",
+    "delay": "^2.0.0",


🤦‍♀️

sindresorhus · 2017-05-06T20:40:04Z

Can you add commits instead of rebasing? Makes it easier to see what changed. We'll squash on merge anyways.

Uses a stream for the body that never closes. Adds a number of console.log expressions for debugging of test control flows.

Adds the dependency and switches out the main got promise for a PCancelable. Adds canceling as soon as the request starts. Disables canceling as soon as the response ends.

alextes · 2017-05-06T20:41:27Z

Sure, will do that in the future. I asked you on Gitter when you squash and when you don't. The only reason I was rebasing is because I wasn't sure if you'd squash.

alextes · 2017-05-06T20:42:29Z

I'll see about reviewing my own PR in the morning next time I'm coding late. Too many slip ups from code that changed heavily during the hour I was trying to find a strong way to write these tests.

alextes · 2017-05-06T20:50:01Z

Alright, ready for re-review @sindresorhus.

alextes · 2017-05-13T20:52:47Z

So I finally finished trying out all ideas I could come up with for solid tests. The in-flight one looks quite solid to me now. It starts a streamed body request, doesn't close the stream until the got triggers the cancel. To make sure we abort on cancel the test then verifies the server emitted an abort event, not a finish event.

The immediate cancel test is much harder. First off, by 'immediate cancel' I mean a user canceling a request before got's underlying event emitter has even emitted its request event. I tried all ideas I had, but the core issue remains. When you cancel a request, you end up with a rejected promise and pretty much lose the opportunity to get any further feedback. After extensive testing, it seems the 'connection' event is the event that hits a Node HTTP server first. This event doesn't fire with an immediate cancel. In other words, unless more of got's internals are exposed it appears impossible to make sure got ever triggered the abort. The compromise I've currently gone with is to make the following assumption: if the server has not received a request within 1000ms, the abort was executed client-side.

Any better method of testing is very welcome, as are of course any pointers to clean up the code more. Sorry for taking so long, this PR has proven to be a very tough one to crack.

sindresorhus · 2017-05-15T18:07:47Z

Any better method of testing is very welcome, as are of course any pointers to clean up the code more.

I can't think of any better way either.

sindresorhus · 2017-05-15T18:10:30Z

This is great stuff @alextes. Really appreciate all the hard work and thoughts you put into it. 🙌

alextes changed the title ~~Enable canceling of in progress requests (WIP)~~ Enable canceling of in progress requests (needs feedback) Apr 1, 2017

alextes force-pushed the abort-request branch 2 times, most recently from 67c7378 to a204087 Compare May 5, 2017 15:39

alextes changed the title ~~Enable canceling of in progress requests (needs feedback)~~ Enable canceling of in progress requests May 5, 2017

alextes force-pushed the abort-request branch from a204087 to 9cdf9e6 Compare May 5, 2017 16:09

sindresorhus reviewed May 5, 2017

View reviewed changes

alextes force-pushed the abort-request branch 3 times, most recently from cc70b02 to 577caff Compare May 6, 2017 00:31

alextes force-pushed the abort-request branch 2 times, most recently from 8b5d113 to ba00120 Compare May 6, 2017 15:40

sindresorhus requested changes May 6, 2017

View reviewed changes

alextes force-pushed the abort-request branch from ba00120 to 210ed8d Compare May 6, 2017 20:23

alextes commented May 6, 2017

View reviewed changes

alextes force-pushed the abort-request branch 2 times, most recently from 90232f5 to e4b008b Compare May 6, 2017 20:38

alextes force-pushed the abort-request branch from e4b008b to 019bff4 Compare May 6, 2017 20:39

alextes added 3 commits May 6, 2017 22:40

Add cancel request test

ecc77ff

Uses a stream for the body that never closes. Adds a number of console.log expressions for debugging of test control flows.

Make promise a PCancelable

9861f00

Adds the dependency and switches out the main got promise for a PCancelable. Adds canceling as soon as the request starts. Disables canceling as soon as the response ends.

Document the abort feature

240c410

alextes force-pushed the abort-request branch from 019bff4 to 240c410 Compare May 6, 2017 20:40

alextes added 4 commits May 6, 2017 22:43

Remove unused error

9c03900

Remove unused dependency

3c5e8f4

Put back double newline

2c3bf27

Use double newline to separate lvl2 headers

c179e0e

Change back lazy evaluation of cancelPromise

cf0f571

sindresorhus requested a review from floatdrop May 7, 2017 11:48

alextes added 3 commits May 13, 2017 19:45

Isolate canceling test

07d0fbd

Add solid in-flight cancel test

3b41aee

Add mediocre immediate cancel test

62f4d3e

alextes changed the title ~~Enable canceling of in progress requests~~ Enable canceling of in progress requests (needs feedback) May 13, 2017

Improve code ordering

a6cb51a

sindresorhus approved these changes May 15, 2017

View reviewed changes

sindresorhus changed the title ~~Enable canceling of in progress requests (needs feedback)~~ Add ability to cancel in-progress requests May 15, 2017

sindresorhus merged commit 9ef7a5a into sindresorhus:master May 15, 2017

alextes deleted the abort-request branch May 15, 2017 19:19

This was referenced May 29, 2017

Update dependency got to version 7.x singapore/gl-got#26

Closed

Update dependency got to version 7.x renovatebot/renovate#223

Merged

		@@ -1,8 +1,13 @@
		import stream from 'stream';
		import PCancelable from 'p-cancelable';

Add ability to cancel in-progress requests #287

Add ability to cancel in-progress requests #287

Conversation

alextes commented Mar 25, 2017 • edited Loading

sindresorhus commented Apr 5, 2017

sindresorhus commented Apr 5, 2017

alextes commented Apr 5, 2017 • edited Loading

sindresorhus commented May 1, 2017

alextes commented May 5, 2017 • edited Loading

alextes commented May 5, 2017

sindresorhus commented May 5, 2017

alextes commented May 5, 2017

alextes commented May 5, 2017 • edited Loading

sindresorhus commented May 5, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented May 6, 2017

alextes commented May 6, 2017

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

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

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

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

sindresorhus commented May 6, 2017

Choose a reason for hiding this comment

sindresorhus commented May 6, 2017

alextes commented May 6, 2017

alextes commented May 6, 2017

alextes commented May 6, 2017

alextes commented May 13, 2017

sindresorhus commented May 15, 2017

sindresorhus commented May 15, 2017

alextes commented Mar 25, 2017 •

edited

Loading

alextes commented Apr 5, 2017 •

edited

Loading

alextes commented May 5, 2017 •

edited

Loading

alextes commented May 5, 2017 •

edited

Loading