Skip to content

Commit

Permalink
use AbortSignal.any if possible
Browse files Browse the repository at this point in the history
  • Loading branch information
tsctx committed Oct 28, 2024
1 parent 2de0f34 commit c904048
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 71 deletions.
108 changes: 62 additions & 46 deletions lib/web/fetch/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ const { kConstruct } = require('../../core/symbols')
const assert = require('node:assert')
const { getMaxListeners, setMaxListeners, defaultMaxListeners } = require('node:events')

// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf
const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23

const kAbortController = Symbol('abortController')

const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => {
Expand Down Expand Up @@ -407,39 +410,44 @@ class Request {
// Realm.
// TODO: could this be simplified with AbortSignal.any
// (https://dom.spec.whatwg.org/#dom-abortsignal-any)
const ac = new AbortController()
this.#signal = ac.signal

// 29. If signal is not null, then make this’s signal follow signal.
if (signal != null) {
if (signal.aborted) {
ac.abort(signal.reason)
} else {
// Keep a strong ref to ac while request object
// is alive. This is needed to prevent AbortController
// from being prematurely garbage collected.
// See, https://github.com/nodejs/undici/issues/1926.
this[kAbortController] = ac

const acRef = new WeakRef(ac)
const abort = buildAbort(acRef)
if (isFixedOrderAbortSignalAny) {
// 29. If signal is not null, then make this’s signal follow signal.
this.#signal = AbortSignal.any(signal != null ? [signal] : [])
} else {
const ac = new AbortController()
this.#signal = ac.signal

// Third-party AbortControllers may not work with these.
// See, https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619.
try {
// If the max amount of listeners is equal to the default, increase it
// This is only available in node >= v19.9.0
if (typeof getMaxListeners === 'function' && getMaxListeners(signal) === defaultMaxListeners) {
setMaxListeners(1500, signal)
}
} catch {}

util.addAbortListener(signal, abort)
// The third argument must be a registry key to be unregistered.
// Without it, you cannot unregister.
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry
// abort is used as the unregister key. (because it is unique)
requestFinalizer.register(ac, { signal, abort }, abort)
// 29. If signal is not null, then make this’s signal follow signal.
if (signal != null) {
if (signal.aborted) {
ac.abort(signal.reason)
} else {
// Keep a strong ref to ac while request object
// is alive. This is needed to prevent AbortController
// from being prematurely garbage collected.
// See, https://github.com/nodejs/undici/issues/1926.
this[kAbortController] = ac

const acRef = new WeakRef(ac)
const abort = buildAbort(acRef)

// Third-party AbortControllers may not work with these.
// See, https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619.
try {
// If the max amount of listeners is equal to the default, increase it
// This is only available in node >= v19.9.0
if (typeof getMaxListeners === 'function' && getMaxListeners(signal) === defaultMaxListeners) {
setMaxListeners(1500, signal)
}
} catch {}

Check failure on line 443 in lib/web/fetch/request.js

View workflow job for this annotation

GitHub Actions / Lint

Trailing spaces not allowed
util.addAbortListener(signal, abort)
// The third argument must be a registry key to be unregistered.
// Without it, you cannot unregister.
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry
// abort is used as the unregister key. (because it is unique)
requestFinalizer.register(ac, { signal, abort }, abort)
}
}
}

Expand Down Expand Up @@ -771,25 +779,33 @@ class Request {
// 3. Let clonedRequestObject be the result of creating a Request object,
// given clonedRequest, this’s headers’s guard, and this’s relevant Realm.
// 4. Make clonedRequestObject’s signal follow this’s signal.
const ac = new AbortController()
if (this.signal.aborted) {
ac.abort(this.signal.reason)
let signal

if (isFixedOrderAbortSignalAny) {
signal = AbortSignal.any([this.#signal])
} else {
let list = dependentControllerMap.get(this.signal)
if (list === undefined) {
list = new Set()
dependentControllerMap.set(this.signal, list)
const ac = new AbortController()
if (this.#signal.aborted) {
ac.abort(this.#signal.reason)
} else {
let list = dependentControllerMap.get(this.#signal)
if (list === undefined) {
list = new Set()
dependentControllerMap.set(this.#signal, list)
}
const acRef = new WeakRef(ac)
list.add(acRef)
util.addAbortListener(
ac.signal,
buildAbort(acRef)
)
}
const acRef = new WeakRef(ac)
list.add(acRef)
util.addAbortListener(
ac.signal,
buildAbort(acRef)
)

signal = ac.signal
}

// 4. Return clonedRequestObject.
return fromInnerRequest(clonedRequest, this.#dispatcher, ac.signal, getHeadersGuard(this.#headers))
return fromInnerRequest(clonedRequest, this.#dispatcher, signal, getHeadersGuard(this.#headers))
}

[nodeUtil.inspect.custom] (depth, options) {
Expand Down
7 changes: 6 additions & 1 deletion test/fetch/issue-node-46525.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,14 @@ const { once } = require('node:events')
const { createServer } = require('node:http')
const { test } = require('node:test')
const { fetch } = require('../..')
const util = require('../../lib/core/util')

// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf
const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23

// TODO: Drop support below node v23, then delete this.
// https://github.com/nodejs/node/issues/46525
test('No warning when reusing AbortController', async (t) => {
test('No warning when reusing AbortController', { skip: isFixedOrderAbortSignalAny }, async (t) => {
function onWarning () {
throw new Error('Got warning')
}
Expand Down
7 changes: 6 additions & 1 deletion test/fetch/long-lived-abort-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,15 @@ const { once } = require('events')
const { test } = require('node:test')
const { closeServerAsPromise } = require('../utils/node-http')
const { strictEqual } = require('node:assert')
const util = require('../../lib/core/util')

// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf
const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23

const isNode18 = process.version.startsWith('v18')

test('long-lived-abort-controller', { skip: isNode18 }, async (t) => {
// TODO: Drop support below node v23, then delete this.
test('long-lived-abort-controller', { skip: isNode18 || isFixedOrderAbortSignalAny }, async (t) => {
const server = http.createServer((req, res) => {
res.writeHead(200, { 'Content-Type': 'text/plain' })
res.write('Hello World!')
Expand Down
7 changes: 6 additions & 1 deletion test/fetch/max-listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,13 @@ const { setMaxListeners, getMaxListeners, defaultMaxListeners } = require('event
const { test } = require('node:test')
const assert = require('node:assert')
const { Request } = require('../..')
const util = require('../../lib/core/util')

test('test max listeners', (t) => {
// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf
const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23

// TODO: Drop support below node v23, then delete this.
test('test max listeners', { skip: isFixedOrderAbortSignalAny }, (t) => {
const controller = new AbortController()
setMaxListeners(Infinity, controller.signal)
for (let i = 0; i <= defaultMaxListeners; i++) {
Expand Down
34 changes: 12 additions & 22 deletions test/fetch/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ const {
fetch
} = require('../../')

const hasSignalReason = 'reason' in AbortSignal.prototype

test('arg validation', async (t) => {
// constructor
assert.throws(() => {
Expand Down Expand Up @@ -261,35 +259,29 @@ test('pre aborted signal', () => {
ac.abort('gwak')
const req = new Request('http://asd', { signal: ac.signal })
assert.strictEqual(req.signal.aborted, true)
if (hasSignalReason) {
assert.strictEqual(req.signal.reason, 'gwak')
}
assert.strictEqual(req.signal.reason, 'gwak')
})

test('post aborted signal', (t) => {
const { strictEqual, ok } = tspl(t, { plan: 2 })
test('post aborted signal', async (t) => {
const { strictEqual, completed } = tspl(t, { plan: 2 })

const ac = new AbortController()
const req = new Request('http://asd', { signal: ac.signal })
strictEqual(req.signal.aborted, false)
ac.signal.addEventListener('abort', () => {
if (hasSignalReason) {
strictEqual(req.signal.reason, 'gwak')
} else {
ok(true)
}
strictEqual(req.signal.reason, 'gwak')
}, { once: true })
ac.abort('gwak')

await completed
})

test('pre aborted signal cloned', () => {
const ac = new AbortController()
ac.abort('gwak')
const req = new Request('http://asd', { signal: ac.signal }).clone()
assert.strictEqual(req.signal.aborted, true)
if (hasSignalReason) {
assert.strictEqual(req.signal.reason, 'gwak')
}
assert.strictEqual(req.signal.reason, 'gwak')
})

test('URLSearchParams body with Headers object - issue #1407', async () => {
Expand All @@ -313,20 +305,18 @@ test('URLSearchParams body with Headers object - issue #1407', async () => {
assert.strictEqual(await request.text(), 'abc=123')
})

test('post aborted signal cloned', (t) => {
const { strictEqual, ok } = tspl(t, { plan: 2 })
test('post aborted signal cloned', async (t) => {
const { strictEqual, completed } = tspl(t, { plan: 2 })

const ac = new AbortController()
const req = new Request('http://asd', { signal: ac.signal }).clone()
strictEqual(req.signal.aborted, false)
ac.signal.addEventListener('abort', () => {
if (hasSignalReason) {
strictEqual(req.signal.reason, 'gwak')
} else {
ok(true)
}
strictEqual(req.signal.reason, 'gwak')
}, { once: true })
ac.abort('gwak')

await completed
})

test('Passing headers in init', async (t) => {
Expand Down

0 comments on commit c904048

Please sign in to comment.