Expose a simple API to abort requests

[delvedor]

With the current API it is not straightforward to abort a request, it would be great to expose a simple method to abort a request.

I was thinking about something very easy, for example:

const request = pool.request({ params }, (err, data) => {
  console.log(err) // RequestAbortedError
})

request.abort()

With promises it should be a thenable.

const request = pool.request({ params })

request
	.then(console.log)
	.catch(console.log) // RequestAbortedError

request.abort()

For supporting this signature, we should transform the returned value in a thenable.
Otherwise, we could create something similar to the AbortController.

What do you think?

Related: #160

[ronag]

@szmarczak @mcollina

[mcollina]

+1!

[ronag]

Then we change so that request no longer returns a Boolean?

Or is the abort controller preferable?

@mcollina does using a thenable instead of a promise in anyway affect async hooks, async await etc...?

[mcollina]

I think using abortcontroller is better.

thenables can break the async context badly :(

[ronag]

I think abortcontroller was discussed in node core somewhere. Will see if I can find that issue.

[mcollina]

See how it is implemented in https://github.com/jasnell/piscina#cancelable-tasks. I think it's a great way to add it.

[delvedor]

I'm going through the code for looking at how to implement the abort controller.
Supporting it is quite easy, in the end it will be something like:

if (opts.signal != null) {
  opts.signal.on('abort', () => {
    callback(new RequestAbortedError(), null)
  })
}

I don't know well enough the internals to know where and what to close/destroy to clean the used resources. @ronag can you point me in the right direction? Or if you want to take a stab, go for it!
I think we should first solve this issue, and then we can work on #165.

[ronag]

I think we should first solve this issue, and then we can work on #165.

Yes, I agree.

Or if you want to take a stab, go for it!

I'll sort it out. If you'd like to build some additional test cases for it afterwards that would be great.

[szmarczak]

In Got we just expose a .cancel() function. It destroys the pending request and makes the promise reject.

[ronag]

In Got we just expose a .cancel() function. It destroys the pending request and makes the promise reject.

What do you expose it on? Do you mean to return a thenable object, i.e. not a Promise?

[szmarczak]

const promise = got('https://example.com'); // This is a promise
promise.cancel(); // The promise will throw an error

[ronag]

@szmarczak Unfortunately that won't work as thenables break async context.

[mcollina]

@szmarczak @ronag that can work if you add a cancel function to the promise object.

[ronag]

that can work if you add a cancel function to the promise object.

Are we fine with modifying the Promise object? I'm a little unsure about it.

[mcollina]

I would try to use the abortcontroller approach instead which is going to be more compatible with the rest of the ecosystem.