Skip to content
Open
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
12 changes: 4 additions & 8 deletions lib/web/fetch/body.js
Original file line number Diff line number Diff line change
Expand Up @@ -422,18 +422,14 @@ function consumeBody (object, convertBytesToJSValue, instance, getInternalState)
return Promise.reject(e)
}

const state = getInternalState(object)
object = getInternalState(object)

// 1. If object is unusable, then return a promise rejected
// with a TypeError.
if (bodyUnusable(state)) {
if (bodyUnusable(object)) {
return Promise.reject(new TypeError('Body is unusable: Body has already been read'))
}

if (state.aborted) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Image

I've wanted to remove this for years

return Promise.reject(new DOMException('The operation was aborted.', 'AbortError'))
}

// 2. Let promise be a new promise.
const promise = createDeferredPromise()

Expand All @@ -454,14 +450,14 @@ function consumeBody (object, convertBytesToJSValue, instance, getInternalState)

// 5. If object’s body is null, then run successSteps with an
// empty byte sequence.
if (state.body == null) {
if (object.body == null) {
successSteps(Buffer.allocUnsafe(0))
return promise.promise
}

// 6. Otherwise, fully read object’s body given successSteps,
// errorSteps, and object’s relevant global object.
fullyReadBody(state.body, successSteps, errorSteps)
fullyReadBody(object.body, successSteps, errorSteps)

// 7. Return promise.
return promise.promise
Expand Down
16 changes: 5 additions & 11 deletions lib/web/fetch/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ function fetch (input, init = undefined) {
if (requestObject.signal.aborted) {
// 1. Abort the fetch() call with p, request, null, and
// requestObject’s signal’s abort reason.
abortFetch(p, request, null, requestObject.signal.reason)
abortFetch(p, request, null, requestObject.signal.reason, null)

// 2. Return p.
return p.promise
Expand Down Expand Up @@ -200,7 +200,7 @@ function fetch (input, init = undefined) {

// 4. Abort the fetch() call with p, request, responseObject,
// and requestObject’s signal’s abort reason.
abortFetch(p, request, realResponse, requestObject.signal.reason)
abortFetch(p, request, realResponse, requestObject.signal.reason, controller.controller)
}
)

Expand All @@ -227,7 +227,7 @@ function fetch (input, init = undefined) {
// 2. Abort the fetch() call with p, request, responseObject, and
// deserializedError.

abortFetch(p, request, responseObject, controller.serializedAbortReason)
abortFetch(p, request, responseObject, controller.serializedAbortReason, controller.controller)
return
}

Expand Down Expand Up @@ -327,7 +327,7 @@ function finalizeAndReportTiming (response, initiatorType = 'other') {
const markResourceTiming = performance.markResourceTiming

// https://fetch.spec.whatwg.org/#abort-fetch
function abortFetch (p, request, responseObject, error) {
function abortFetch (p, request, responseObject, error, controller /* undici-specific */) {
// 1. Reject promise with error.
if (p) {
// We might have already resolved the promise at this stage
Expand Down Expand Up @@ -357,13 +357,7 @@ function abortFetch (p, request, responseObject, error) {
// 5. If response’s body is not null and is readable, then error response’s
// body with error.
if (response.body?.stream != null && isReadable(response.body.stream)) {
response.body.stream.cancel(error).catch((err) => {
if (err.code === 'ERR_INVALID_STATE') {
// Node bug?
return
}
throw err
})
controller.error(error)
}
}

Expand Down
109 changes: 109 additions & 0 deletions test/fetch/issue-4799.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
'use strict'

const { test } = require('node:test')
const { fetch } = require('../..')
const { createServer } = require('node:http')
const { once } = require('node:events')
const { closeServerAsPromise } = require('../utils/node-http')

test('response clone + abort should return AbortError, not TypeError', async (t) => {
const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
res.statusCode = 200
res.setHeader('Content-Type', 'application/json')
res.end(JSON.stringify({ message: 'hello' }))
})

t.after(closeServerAsPromise(server))
server.listen(0)
await once(server, 'listening')

const controller = new AbortController()
const response = await fetch(`http://localhost:${server.address().port}`, {
signal: controller.signal
})

// Clone the response before aborting
const clonedResponse = response.clone()

// Abort after cloning
controller.abort()

// Both original and cloned response should reject with AbortError
await t.test('original response should reject with AbortError', async () => {
await t.assert.rejects(
response.text(),
{
name: 'AbortError',
code: DOMException.ABORT_ERR
}
)
})

await t.test('cloned response should reject with AbortError', async () => {
await t.assert.rejects(
clonedResponse.text(),
{
name: 'AbortError',
code: DOMException.ABORT_ERR
}
)
})
})

test('response without clone + abort should still return AbortError', async (t) => {
const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
res.statusCode = 200
res.setHeader('Content-Type', 'application/json')
res.end(JSON.stringify({ message: 'hello' }))
})

t.after(closeServerAsPromise(server))
server.listen(0)
await once(server, 'listening')

const controller = new AbortController()
const response = await fetch(`http://localhost:${server.address().port}`, {
signal: controller.signal
})

// Abort without cloning
controller.abort()

await t.assert.rejects(
response.text(),
{
name: 'AbortError',
code: DOMException.ABORT_ERR
}
)
})

test('response bodyUsed after clone and abort', async (t) => {
const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
res.statusCode = 200
res.setHeader('Content-Type', 'application/json')
res.end(JSON.stringify({ message: 'hello' }))
})

t.after(closeServerAsPromise(server))
server.listen(0)
await once(server, 'listening')

const controller = new AbortController()
const response = await fetch(`http://localhost:${server.address().port}`, {
signal: controller.signal
})

t.assert.strictEqual(response.bodyUsed, false)

const clonedResponse = response.clone()

t.assert.strictEqual(response.bodyUsed, false)
t.assert.strictEqual(clonedResponse.bodyUsed, false)

controller.abort()

// Erroring a stream (see: https://fetch.spec.whatwg.org/#abort-fetch step 5) does not disturb it.
t.assert.strictEqual(response.bodyUsed, false)
Copy link
Member Author

Choose a reason for hiding this comment

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

In #4801 Claude, for some reason, thought bodyUsed was true here. All of the other tests are unchanged.

t.assert.strictEqual(clonedResponse.bodyUsed, false)
})
Loading