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.

Sign up for GitHub

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

Jump to bottom

use AbortSignal.any if possible #3779

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

use AbortSignal.any if possible #3779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c904048
use AbortSignal.any if possible
tsctx Oct 28, 2024
906a88e
lint fix
tsctx Oct 28, 2024
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
108 changes: 62 additions & 46 deletions lib/web/fetch/request.js
View file
Open in desktop
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 {}

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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,15 @@
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 Expand Up @@ -44,5 +49,5 @@
await res.text()
}

strictEqual(warningEmitted, false)

Check failure on line 52 in test/fetch/long-lived-abort-controller.js

View workflow job for this annotation

GitHub Actions / test (22, ubuntu-latest) / Test with Node.js 22 on ubuntu-latest

long-lived-abort-controller 

[Error [ERR_TEST_FAILURE]: Expected values to be strictly equal:

true !== false
] {
  code: 'ERR_TEST_FAILURE',
  failureType: 'testCodeFailure',
  cause: AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  
  true !== false
  
      at TestContext.<anonymous> (/home/runner/work/undici/undici/test/fetch/long-lived-abort-controller.js:52:3)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async Test.run (node:internal/test_runner/test:935:9)
      at async startSubtestAfterBootstrap (node:internal/test_runner/harness:296:3) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: true,
    expected: false,
    operator: 'strictEqual'
  }
}

Check failure on line 52 in test/fetch/long-lived-abort-controller.js

View workflow job for this annotation

GitHub Actions / Test with Node.js 22 compiled --without-intl

long-lived-abort-controller 

[Error [ERR_TEST_FAILURE]: Expected values to be strictly equal:

true !== false
] {
  code: 'ERR_TEST_FAILURE',
  failureType: 'testCodeFailure',
  cause: AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  
  true !== false
  
      at TestContext.<anonymous> (/home/runner/work/undici/undici/test/fetch/long-lived-abort-controller.js:52:3)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async Test.run (node:internal/test_runner/test:935:9)
      at async startSubtestAfterBootstrap (node:internal/test_runner/harness:296:3) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: true,
    expected: false,
    operator: 'strictEqual'
  }
}
})
7 changes: 6 additions & 1 deletion test/fetch/max-listeners.js
View file
Open in desktop
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
View file
Open in desktop
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
Loading