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: handle request body as late as possible #2734

Merged
merged 2 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
36 changes: 33 additions & 3 deletions lib/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ const {
// Experimental
let h2ExperimentalWarned = false

let extractBody

const FastBuffer = Buffer[Symbol.species]

const kClosedResolve = Symbol('kClosedResolve')
Expand Down Expand Up @@ -1446,7 +1448,7 @@ function _resume (client, sync) {
}

if (client[kRunning] > 0 && util.bodyLength(request.body) !== 0 &&
(util.isStream(request.body) || util.isAsyncIterable(request.body))) {
(util.isStream(request.body) || util.isAsyncIterable(request.body) || util.isFormDataLike(request.body))) {
// Request with stream or iterator body can error while other requests
// are inflight and indirectly error those as well.
// Ensure this doesn't happen by waiting for inflight
Expand Down Expand Up @@ -1477,7 +1479,9 @@ function write (client, request) {
return
}

const { body, method, path, host, upgrade, headers, blocking, reset } = request
const { method, path, host, upgrade, blocking, reset } = request

let { body, headers, contentLength } = request

// https://tools.ietf.org/html/rfc7231#section-4.3.1
// https://tools.ietf.org/html/rfc7231#section-4.3.2
Expand All @@ -1494,14 +1498,29 @@ function write (client, request) {
method === 'PATCH'
)

if (util.isFormDataLike(body)) {
Copy link
Member

Choose a reason for hiding this comment

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

For another PR, we will need to port this to H2 as well

if (!extractBody) {
extractBody = require('./fetch/body.js').extractBody
}

const [bodyStream, contentType] = extractBody(body)
if (request.contentType == null) {
ronag marked this conversation as resolved.
Show resolved Hide resolved
headers += `content-type: ${contentType}\r\n`
}
body = bodyStream.stream
contentLength = bodyStream.length
} else if (util.isBlobLike(body) && request.contentType == null && body.type) {
headers += `content-type: ${body.type}\r\n`
}

if (body && typeof body.read === 'function') {
// Try to read EOF in order to get length.
body.read(0)
}

const bodyLength = util.bodyLength(body)

let contentLength = bodyLength
contentLength = bodyLength ?? contentLength

if (contentLength === null) {
contentLength = request.contentLength
Expand Down Expand Up @@ -1544,6 +1563,7 @@ function write (client, request) {
}

if (request.aborted) {
util.destroy(body)
return false
}

Expand Down Expand Up @@ -2050,6 +2070,16 @@ function writeStream ({ h2stream, body, client, request, socket, contentLength,
socket
.on('drain', onDrain)
.on('error', onFinished)

if (body.errored) {
queueMicrotask(() => onFinished(body.errored))
} else if (body.readableEnded) {
queueMicrotask(() => onFinished(null))
}

if (body.closeEmitted ?? body.closed) {
queueMicrotask(onClose)
}
}

async function writeBlob ({ h2stream, body, client, request, socket, contentLength, header, expectsPayload }) {
Expand Down
19 changes: 0 additions & 19 deletions lib/core/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ const invalidPathRegex = /[^\u0021-\u00ff]/

const kHandler = Symbol('handler')

let extractBody

class Request {
constructor (origin, {
path,
Expand Down Expand Up @@ -173,23 +171,6 @@ class Request {
throw new InvalidArgumentError('headers must be an object or an array')
}

if (util.isFormDataLike(this.body)) {
if (!extractBody) {
extractBody = require('../fetch/body.js').extractBody
}

const [bodyStream, contentType] = extractBody(body)
if (this.contentType == null) {
this.contentType = contentType
this.headers += `content-type: ${contentType}\r\n`
}
this.body = bodyStream.stream
this.contentLength = bodyStream.length
} else if (util.isBlobLike(body) && this.contentType == null && body.type) {
this.contentType = body.type
this.headers += `content-type: ${body.type}\r\n`
}

util.validateHandler(handler, method, upgrade)

this.servername = util.getServerName(this.host)
Expand Down
4 changes: 2 additions & 2 deletions lib/core/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,8 @@ function bodyLength (body) {
return null
}

function isDestroyed (stream) {
return !stream || !!(stream.destroyed || stream[kDestroyed])
function isDestroyed (body) {
return body && !!(body.destroyed || body[kDestroyed] || (stream.isDestroyed?.(body)))
}

function isReadableAborted (stream) {
Expand Down
1 change: 1 addition & 0 deletions test/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,7 @@ test('basic POST with empty stream', (t) => {
method: 'POST',
body
}, (err, { statusCode, headers, body }) => {
console.error(err)
Copy link
Member

Choose a reason for hiding this comment

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

Left over?

t.error(err)
body
.on('data', () => {
Expand Down
Loading