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(fetch): use AbortError DOMException #1511

Merged
merged 2 commits into from
Jun 27, 2022
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions lib/core/errors.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,5 @@
'use strict'

class AbortError extends Error {
constructor () {
super('The operation was aborted')
this.code = 'ABORT_ERR'
this.name = 'AbortError'
}
}

class UndiciError extends Error {
constructor (message) {
super(message)
Expand Down Expand Up @@ -191,7 +183,6 @@ class HTTPParserError extends Error {
}

module.exports = {
AbortError,
HTTPParserError,
UndiciError,
HeadersTimeoutError,
Expand Down
12 changes: 12 additions & 0 deletions lib/fetch/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,19 @@ const subresource = [
''
]

/** @type {globalThis['DOMException']} */
const DOMException = globalThis.DOMException ?? (() => {
Copy link
Member

Choose a reason for hiding this comment

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

We should have a DomException in Node, no need for polyfill.

Copy link
Member Author

Choose a reason for hiding this comment

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

Somehow missed this notification 😕.

DOMException is available in node globally from v17 and above, but fetch supports v16.5 and above. There's a comment detailing why this is needed a few lines below.

Copy link
Member

Choose a reason for hiding this comment

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

You can get it with:

try {
  atob('~')
} catch (err) {
  const DOMException = Object.getPrototypeOf(err).constructor

  console.log(new DOMException('aaa'))
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in edc4f54

// DOMException was only made a global in Node v17.0.0,
// but fetch supports >= v16.5.
try {
atob('~')
} catch (err) {
return Object.getPrototypeOf(err).constructor
}
})()

module.exports = {
DOMException,
subresource,
forbiddenMethods,
requestBodyHeader,
Expand Down
16 changes: 9 additions & 7 deletions lib/fetch/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,15 @@ const {
isAborted
} = require('./util')
const { kState, kHeaders, kGuard, kRealm } = require('./symbols')
const { AbortError } = require('../core/errors')
const assert = require('assert')
const { safelyExtractBody, extractBody } = require('./body')
const {
redirectStatus,
nullBodyStatus,
safeMethods,
requestBodyHeader,
subresource
subresource,
DOMException
} = require('./constants')
const { kHeadersList } = require('../core/symbols')
const EE = require('events')
Expand Down Expand Up @@ -82,7 +82,7 @@ class Fetch extends EE {
return
}

const reason = new AbortError()
const reason = new DOMException('The operation was aborted.', 'AbortError')

this.state = 'aborted'
this.connection?.destroy(reason)
Expand Down Expand Up @@ -295,7 +295,7 @@ function markResourceTiming () {
// https://fetch.spec.whatwg.org/#abort-fetch
function abortFetch (p, request, responseObject) {
// 1. Let error be an "AbortError" DOMException.
const error = new AbortError()
const error = new DOMException('The operation was aborted.', 'AbortError')

// 2. Reject promise with error.
p.reject(error)
Expand Down Expand Up @@ -1555,7 +1555,7 @@ async function httpNetworkFetch (
destroy (err) {
if (!this.destroyed) {
this.destroyed = true
this.abort?.(err ?? new AbortError())
this.abort?.(err ?? new DOMException('The operation was aborted.', 'AbortError'))
}
}
}
Expand Down Expand Up @@ -1885,7 +1885,9 @@ async function httpNetworkFetch (

// 2. If stream is readable, error stream with an "AbortError" DOMException.
if (isReadable(stream)) {
fetchParams.controller.controller.error(new AbortError())
fetchParams.controller.controller.error(
new DOMException('The operation was aborted.', 'AbortError')
)
}
} else {
// 3. Otherwise, if stream is readable, error stream with a TypeError.
Expand Down Expand Up @@ -1926,7 +1928,7 @@ async function httpNetworkFetch (
const { connection } = fetchParams.controller

if (connection.destroyed) {
abort(new AbortError())
abort(new DOMException('The operation was aborted.', 'AbortError'))
} else {
fetchParams.controller.on('terminated', abort)
this.abort = connection.abort = abort
Expand Down
6 changes: 3 additions & 3 deletions lib/fetch/response.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict'

const { Headers, HeadersList, fill } = require('./headers')
const { AbortError } = require('../core/errors')
const { extractBody, cloneBody, mixinBody } = require('./body')
const util = require('../core/util')
const { kEnumerableProperty } = util
Expand All @@ -15,7 +14,8 @@ const {
} = require('./util')
const {
redirectStatus,
nullBodyStatus
nullBodyStatus,
DOMException
} = require('./constants')
const { kState, kHeaders, kGuard, kRealm } = require('./symbols')
const { webidl } = require('./webidl')
Expand Down Expand Up @@ -440,7 +440,7 @@ function makeAppropriateNetworkError (fetchParams) {
// 2. Return an aborted network error if fetchParams is aborted;
// otherwise return a network error.
return isAborted(fetchParams)
? makeNetworkError(new AbortError())
? makeNetworkError(new DOMException('The operation was aborted.', 'AbortError'))
: makeNetworkError(fetchParams.controller.terminated.reason)
}

Expand Down
51 changes: 50 additions & 1 deletion test/fetch/abort.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ const { test } = require('tap')
const { fetch } = require('../..')
const { createServer } = require('http')
const { once } = require('events')
const { ReadableStream } = require('stream/web')
const { DOMException } = require('../../lib/fetch/constants')

/* global AbortController */

Expand Down Expand Up @@ -52,8 +54,55 @@ test('parallel fetch with the same AbortController works as expected', async (t)
t.equal(rejected.length, 9) // out of 10 requests, only 1 should succeed
t.equal(resolved.length, 1)

t.ok(rejected.every(rej => rej.reason?.code === 'ABORT_ERR'))
t.ok(rejected.every(rej => rej.reason?.code === DOMException.ABORT_ERR))
t.same(resolved[0].value, body)

t.end()
})

// https://github.com/web-platform-tests/wpt/blob/fd8aeb1bb2eb33bc43f8a5bbc682b0cff6075dfe/fetch/api/abort/general.any.js#L474-L507
test('Readable stream synchronously cancels with AbortError if aborted before reading', async (t) => {
const server = createServer((req, res) => {
res.write('')
res.end()
}).listen(0)

t.teardown(server.close.bind(server))
await once(server, 'listening')

const controller = new AbortController()
const signal = controller.signal
controller.abort()

let cancelReason

const body = new ReadableStream({
pull (controller) {
controller.enqueue(new Uint8Array([42]))
},
cancel (reason) {
cancelReason = reason
}
})

const fetchPromise = fetch(`http://localhost:${server.address().port}`, {
body,
signal,
method: 'POST',
headers: {
'Content-Type': 'text/plain'
}
})

t.ok(cancelReason, 'Cancel called sync')
t.equal(cancelReason.constructor, DOMException)
t.equal(cancelReason.name, 'AbortError')

await t.rejects(fetchPromise, { name: 'AbortError' })

const fetchErr = await fetchPromise.catch(e => e)

t.equal(cancelReason, fetchErr, 'Fetch rejects with same error instance')

t.end()
})