diff --git a/packages/aws-lambda/README.md b/packages/aws-lambda/README.md index 6b4cb1ac..8db0f598 100644 --- a/packages/aws-lambda/README.md +++ b/packages/aws-lambda/README.md @@ -86,7 +86,7 @@ export const handler = awslambda.streamifyResponse(async (event, responseStream, }) ``` -`sendStandardResponse()` sends the status, headers, and cookies as the response stream metadata prelude via `awslambda.HttpResponseStream.from()`, then streams the body. It resolves once the response is fully flushed, and rejects if the stream errors. +`sendStandardResponse()` sends the status, headers, and cookies as the response stream metadata prelude via `awslambda.HttpResponseStream.from()`, then streams the body. It resolves once the response is fully flushed or the client disconnects, and rejects if the stream fails otherwise. > [!TIP] > When sending responses, you can pass additional options such as event-stream keep-alive. diff --git a/packages/aws-lambda/src/response.test.ts b/packages/aws-lambda/src/response.test.ts index c3b0790d..ce36c6fe 100644 --- a/packages/aws-lambda/src/response.test.ts +++ b/packages/aws-lambda/src/response.test.ts @@ -48,6 +48,10 @@ function createResponseStream(): HttpResponseStream & { chunks: Buffer[] } { return stream } +function createDisconnectError(): Error { + return Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }) +} + function metadataOf(stream: HttpResponseStream & { chunks: Buffer[] }): Record { return JSON.parse(stream.chunks[0]!.toString()) } @@ -253,40 +257,46 @@ describe('sendStandardResponse', () => { await sendPromise }) - it('destroys the body without error when the response stream closes cleanly while streaming', async () => { - const responseStream = createResponseStream() + it.each([ + ['cleanly', undefined], + ['on a client disconnect', createDisconnectError()], + ])( + 'resolves and destroys the body when the response stream closes %s while streaming', + async (_, error) => { + const responseStream = createResponseStream() - let clean = false - const standardResponse: StandardResponse = { - body: (async function* () { - try { - yield 1 - await new Promise((r) => setTimeout(r, 100)) - yield 2 - await new Promise((r) => setTimeout(r, 9999999)) - yield 3 - } finally { - clean = true - } - })(), - headers: {}, - status: 200, - } + let clean = false + const standardResponse: StandardResponse = { + body: (async function* () { + try { + yield 1 + await new Promise((r) => setTimeout(r, 100)) + yield 2 + await new Promise((r) => setTimeout(r, 9999999)) + yield 3 + } finally { + clean = true + } + })(), + headers: {}, + status: 200, + } - const sendPromise = sendStandardResponse(responseStream, standardResponse) + const sendPromise = sendStandardResponse(responseStream, standardResponse) - await vi.waitFor(() => { - expect(bodyOf(responseStream)).toContain('data: 1') - }) + await vi.waitFor(() => { + expect(bodyOf(responseStream)).toContain('data: 1') + }) - responseStream.destroy() + responseStream.destroy(error) - await vi.waitFor(() => { - expect(clean).toBe(true) - }) + await vi.waitFor(() => { + expect(clean).toBe(true) + }) - await sendPromise - }) + await expect(sendPromise).resolves.toBeUndefined() + }, + ) it.each([ [ @@ -329,9 +339,13 @@ describe('sendStandardResponse', () => { }) describe('response stream closed before sending', () => { - it('resolves and destroys the body', async () => { + it.each([ + ['closed cleanly', undefined], + ['destroyed by a client disconnect', createDisconnectError()], + ])('resolves and destroys the body when the response stream was %s', async (_, error) => { const responseStream = createResponseStream() - responseStream.destroy() + responseStream.once('error', () => {}) + responseStream.destroy(error) await vi.waitFor(() => { expect(responseStream.closed).toBe(true) diff --git a/packages/aws-lambda/src/response.ts b/packages/aws-lambda/src/response.ts index 3376ef03..770edfc9 100644 --- a/packages/aws-lambda/src/response.ts +++ b/packages/aws-lambda/src/response.ts @@ -4,6 +4,7 @@ import { canWriteToNodeResponse, destroyNodeHttpBody, getNodeResponseError, + isNodeResponseDisconnectError, toNodeHttpBody, } from '@standard-server/node' @@ -40,7 +41,7 @@ export async function sendStandardResponse( destroyNodeHttpBody(resBody, error, reject) - if (error) { + if (error && !isNodeResponseDisconnectError(error)) { reject(error) } else { resolve() @@ -49,7 +50,11 @@ export async function sendStandardResponse( return } - responseStream.once('error', reject) + responseStream.once('error', (error) => { + if (!isNodeResponseDisconnectError(error)) { + reject(error) + } + }) responseStream.once('close', resolve) try { diff --git a/packages/fastify/README.md b/packages/fastify/README.md index a598d7a1..8e05d7bc 100644 --- a/packages/fastify/README.md +++ b/packages/fastify/README.md @@ -90,7 +90,7 @@ fastify.all('/*', async (req, reply) => { await fastify.listen({ port: 3000 }) ``` -`sendStandardResponse()` resolves once the response is fully flushed, and rejects if the underlying connection errors. Do not return a value from the route handler afterwards — Fastify would try to send a second response. +`sendStandardResponse()` resolves once the response is fully flushed or the client disconnects (an HTTP/2 stream reset included), and rejects if sending fails otherwise. Do not return a value from the route handler afterwards — Fastify would try to send a second response. > [!TIP] > When sending responses, you can pass additional options such as event-stream keep-alive. diff --git a/packages/fastify/src/response.test.ts b/packages/fastify/src/response.test.ts index e0500eb5..efba3a74 100644 --- a/packages/fastify/src/response.test.ts +++ b/packages/fastify/src/response.test.ts @@ -733,34 +733,86 @@ describe('sendStandardResponse', () => { }) }) - it('rejects with the stream error when the client resets an http2 stream with an error code', async ({ - onTestFinished, - }) => { - const { body, isReleased } = createEndlessBody() - let sending: Promise | undefined + describe('http2 client disconnect', () => { + const disconnects: [ + string, + (client: http2.ClientHttp2Session, reqStream: http2.ClientHttp2Stream) => void, + ][] = [ + ['an error reset', (_, reqStream) => reqStream.close(http2.constants.NGHTTP2_INTERNAL_ERROR)], + ['an error goaway', (client) => client.goaway(http2.constants.NGHTTP2_PROTOCOL_ERROR)], + ] + + it.for(disconnects)( + 'resolves and releases the body on %s while sending', + async ([, disconnect], { onTestFinished }) => { + const { body, isReleased } = createEndlessBody() + let sending: Promise | undefined + + const fastify = Fastify({ http2: true }) + onTestFinished(() => fastify.close()) + + fastify.get('/', async (req, reply) => { + sending = sendStandardResponse(reply, { status: 200, headers: {}, body }) + await sending + }) - const fastify = Fastify({ http2: true }) - onTestFinished(() => fastify.close()) + const client = http2.connect(await fastify.listen({ port: 0, host: '127.0.0.1' })) + client.once('error', () => {}) + onTestFinished(() => client.close()) - fastify.get('/', async (req, reply) => { - sending = sendStandardResponse(reply, { status: 200, headers: {}, body }) - await sending.catch(() => {}) - }) + const reqStream = client.request({ ':path': '/' }) + reqStream.once('error', () => {}) + + await new Promise((r) => reqStream.once('data', r)) + disconnect(client, reqStream) - const client = http2.connect(await fastify.listen({ port: 0, host: '127.0.0.1' })) - onTestFinished(() => client.close()) + await expect(sending).resolves.toBeUndefined() - const reqStream = client.request({ ':path': '/' }) - reqStream.once('error', () => {}) + await vi.waitFor(() => { + expect(isReleased()).toBe(true) + }) + }, + ) - await new Promise((r) => reqStream.once('data', r)) - reqStream.close(http2.constants.NGHTTP2_INTERNAL_ERROR) + it.for(disconnects)( + 'resolves and releases the body on %s before sending', + async ([, disconnect], { onTestFinished }) => { + const { body, isReleased } = createEndlessBody() - await expect(sending).rejects.toMatchObject({ code: 'ERR_HTTP2_STREAM_ERROR' }) + let received!: () => void + const receivedPromise = new Promise((r) => (received = r)) + let send!: (sending: Promise) => void + const sending = new Promise((r) => (send = r)) - await vi.waitFor(() => { - expect(isReleased()).toBe(true) - }) + const fastify = Fastify({ http2: true }) + onTestFinished(() => fastify.close()) + + fastify.get('/', async (req, reply) => { + const closed = new Promise((r) => reply.raw.stream.once('close', r)) + received() + await closed + + send(sendStandardResponse(reply, { status: 200, headers: {}, body })) + await sending + }) + + const client = http2.connect(await fastify.listen({ port: 0, host: '127.0.0.1' })) + client.once('error', () => {}) + onTestFinished(() => client.close()) + + const reqStream = client.request({ ':path': '/' }) + reqStream.once('error', () => {}) + + await receivedPromise + disconnect(client, reqStream) + + await expect(sending).resolves.toBeUndefined() + + await vi.waitFor(() => { + expect(isReleased()).toBe(true) + }) + }, + ) }) }) diff --git a/packages/fastify/src/response.ts b/packages/fastify/src/response.ts index fa46d439..cdfd16da 100644 --- a/packages/fastify/src/response.ts +++ b/packages/fastify/src/response.ts @@ -4,6 +4,7 @@ import { canWriteToNodeResponse, destroyNodeHttpBody, getNodeResponseError, + isNodeResponseDisconnectError, toNodeHttpBody, } from '@standard-server/node' @@ -28,7 +29,7 @@ export async function sendStandardResponse( destroyNodeHttpBody(resBody, error, reject) - if (error) { + if (error && !isNodeResponseDisconnectError(error)) { reject(error) } else { resolve() @@ -39,7 +40,11 @@ export async function sendStandardResponse( const connection = 'stream' in reply.raw ? reply.raw.stream : reply.raw - connection.once('error', reject) + connection.once('error', (error) => { + if (!isNodeResponseDisconnectError(error)) { + reject(error) + } + }) connection.once('close', () => { if (typeof resBody === 'object' && !resBody.closed) { resBody.destroy() diff --git a/packages/node/README.md b/packages/node/README.md index 47ec6058..13f523b6 100644 --- a/packages/node/README.md +++ b/packages/node/README.md @@ -85,6 +85,8 @@ createServer(async (req, res) => { }).listen(3000) ``` +`sendStandardResponse()` resolves once the response is fully flushed or the client disconnects (an HTTP/2 stream reset included), and rejects if sending fails otherwise. + > [!TIP] > When sending responses, you can pass additional options such as event-stream keep-alive. diff --git a/packages/node/src/response.test.ts b/packages/node/src/response.test.ts index 096ec4ce..b80b8067 100644 --- a/packages/node/src/response.test.ts +++ b/packages/node/src/response.test.ts @@ -2,10 +2,12 @@ import { Buffer } from 'node:buffer' import type { IncomingMessage, ServerResponse } from 'node:http' import type { Http2ServerRequest, Http2ServerResponse } from 'node:http2' import http2 from 'node:http2' -import type { AddressInfo } from 'node:net' +import type { AddressInfo, Socket } from 'node:net' +import { connect } from 'node:net' import Stream from 'node:stream' import type { StandardResponse } from '@standard-server/core' +import { promiseWithResolvers } from '@standard-server/shared' import request from 'supertest' import * as Body from './body' @@ -575,17 +577,7 @@ describe('sendStandardResponse', () => { }) it('rejects and releases a stream body it cannot send', async () => { - let clean = false - const body = (async function* () { - try { - while (true) { - yield 'foo' - await new Promise((r) => setTimeout(r, 10)) - } - } finally { - clean = true - } - })() + const { body, isReleased } = createEndlessBody() let sending: Promise | undefined @@ -603,65 +595,131 @@ describe('sendStandardResponse', () => { expect(response.body).toBe('') await vi.waitFor(() => { - expect(clean).toBe(true) + expect(isReleased()).toBe(true) }) }) }) - describe('http2 stream reset by the client', () => { - it.each([ - ['resolves on a cancel', http2.constants.NGHTTP2_CANCEL, undefined], + describe('http2 client disconnect', () => { + const disconnects: [string, Http2Disconnect][] = [ + ['a cancel reset', ({ reqStream }) => reqStream.close(http2.constants.NGHTTP2_CANCEL)], [ - 'rejects with the stream error on an error code', - http2.constants.NGHTTP2_INTERNAL_ERROR, - 'ERR_HTTP2_STREAM_ERROR', + 'a refused stream reset', + ({ reqStream }) => reqStream.close(http2.constants.NGHTTP2_REFUSED_STREAM), ], - ])('%s', async (_, code, errorCode) => { - let clean = false - const body = (async function* () { - try { - while (true) { - yield 'foo' - await new Promise((r) => setTimeout(r, 10)) - } - } finally { - clean = true - } - })() - - let sending: Promise | undefined + [ + 'an internal error reset', + ({ reqStream }) => reqStream.close(http2.constants.NGHTTP2_INTERNAL_ERROR), + ], + [ + 'a protocol error goaway', + ({ client }) => client.goaway(http2.constants.NGHTTP2_PROTOCOL_ERROR), + ], + ['a connection reset', ({ socket }) => socket.resetAndDestroy()], + ] - const server = http2.createServer((req, res) => { - sending = sendStandardResponse(res, { status: 200, headers: {}, body }) - }) - await new Promise((r) => server.listen(0, r)) + it.for(disconnects)( + 'resolves and releases the body on %s while sending', + async ([, disconnect], { onTestFinished }) => { + const { body, isReleased } = createEndlessBody() + let sending: Promise | undefined - const client = http2.connect(`http://localhost:${(server.address() as AddressInfo).port}`) + const { client, socket } = await connectHttp2(onTestFinished, (req, res) => { + sending = sendStandardResponse(res, { status: 200, headers: {}, body }) + }) - try { const reqStream = client.request({ ':path': '/' }) reqStream.once('error', () => {}) await new Promise((r) => reqStream.once('data', r)) - reqStream.close(code) + disconnect({ client, reqStream, socket }) - if (errorCode) { - await expect(sending).rejects.toMatchObject({ code: errorCode }) - } else { - await expect(sending).resolves.toBeUndefined() - } + await expect(sending).resolves.toBeUndefined() await vi.waitFor(() => { - expect(clean).toBe(true) + expect(isReleased()).toBe(true) }) - } finally { - client.close() - await new Promise((r) => server.close(r)) - } - }) + }, + ) + + it.for(disconnects)( + 'resolves and releases the body on %s before sending', + async ([, disconnect], { onTestFinished }) => { + const { body, isReleased } = createEndlessBody() + const received = promiseWithResolvers() + const sending = promiseWithResolvers>() + + const { client, socket } = await connectHttp2(onTestFinished, (req, res) => { + res.stream.once('close', () => { + sending.resolve(sendStandardResponse(res, { status: 200, headers: {}, body })) + }) + + received.resolve() + }) + + const reqStream = client.request({ ':path': '/' }) + reqStream.once('error', () => {}) + + await received.promise + // a round trip first, or the server can see a connection reset as a plain close + await new Promise((r) => client.ping(r)) + disconnect({ client, reqStream, socket }) + + await expect(sending.promise).resolves.toBeUndefined() + + await vi.waitFor(() => { + expect(isReleased()).toBe(true) + }) + }, + ) }) }) +type Http2Disconnect = (connection: { + client: http2.ClientHttp2Session + reqStream: http2.ClientHttp2Stream + socket: Socket +}) => void + +async function connectHttp2( + onTestFinished: (fn: () => Promise) => void, + listener: (req: Http2ServerRequest, res: Http2ServerResponse) => void, +): Promise<{ client: http2.ClientHttp2Session; socket: Socket }> { + const server = http2.createServer(listener) + await new Promise((r) => server.listen(0, r)) + + const port = (server.address() as AddressInfo).port + let socket!: Socket + const client = http2.connect(`http://localhost:${port}`, { + createConnection: () => (socket = connect(port, 'localhost')), + }) + client.once('error', () => {}) + + onTestFinished(async () => { + client.destroy() + await new Promise((r) => server.close(r)) + }) + + return { client, socket } +} + +function createEndlessBody() { + let released = false + + const body = (async function* () { + try { + while (true) { + yield 'foo' + await new Promise((r) => setTimeout(r, 10)) + } + } finally { + released = true + } + })() + + return { body, isReleased: () => released } +} + async function requestHttp2Head( listener: (req: Http2ServerRequest, res: Http2ServerResponse) => void, ): Promise<{ headers: http2.IncomingHttpHeaders; body: string }> { diff --git a/packages/node/src/response.ts b/packages/node/src/response.ts index 47a7fafe..9ca44309 100644 --- a/packages/node/src/response.ts +++ b/packages/node/src/response.ts @@ -3,7 +3,12 @@ import type { StandardResponse } from '@standard-server/core' import type { ToNodeHttpBodyOptions } from './body' import { toNodeHttpBody } from './body' import type { NodeHttpResponse } from './types' -import { canWriteToNodeResponse, destroyNodeHttpBody, getNodeResponseError } from './utils' +import { + canWriteToNodeResponse, + destroyNodeHttpBody, + getNodeResponseError, + isNodeResponseDisconnectError, +} from './utils' export interface SendStandardResponseOptions extends ToNodeHttpBodyOptions {} @@ -24,7 +29,7 @@ export async function sendStandardResponse( destroyNodeHttpBody(resBody, error, reject) - if (error) { + if (error && !isNodeResponseDisconnectError(error)) { reject(error) } else { resolve() @@ -35,7 +40,11 @@ export async function sendStandardResponse( const connection = 'stream' in res ? res.stream : res - connection.once('error', reject) + connection.once('error', (error) => { + if (!isNodeResponseDisconnectError(error)) { + reject(error) + } + }) connection.once('close', resolve) try { diff --git a/packages/node/src/utils.test.ts b/packages/node/src/utils.test.ts index 3eee7e0a..84e2e00f 100644 --- a/packages/node/src/utils.test.ts +++ b/packages/node/src/utils.test.ts @@ -13,6 +13,7 @@ import { canWriteToNodeResponse, destroyNodeHttpBody, getNodeResponseError, + isNodeResponseDisconnectError, toWebReadableStream, } from './utils' @@ -349,6 +350,25 @@ describe('getNodeResponseError', () => { }) }) +describe('isNodeResponseDisconnectError', () => { + it.each(['ERR_HTTP2_STREAM_ERROR', 'ERR_HTTP2_SESSION_ERROR', 'ECONNRESET', 'EPIPE'])( + 'on an error with code %s', + (code) => { + expect(isNodeResponseDisconnectError(Object.assign(new Error('test'), { code }))).toBe(true) + }, + ) + + it.each([ + ['an error without a code', new Error('test')], + [ + 'an error with another code', + Object.assign(new Error('test'), { code: 'ERR_STREAM_WRITE_AFTER_END' }), + ], + ])('on %s', (_, error) => { + expect(isNodeResponseDisconnectError(error)).toBe(false) + }) +}) + describe('destroyNodeHttpBody', () => { it('ignores non-stream bodies', () => { const onError = vi.fn() diff --git a/packages/node/src/utils.ts b/packages/node/src/utils.ts index 459a1833..f9fc74c2 100644 --- a/packages/node/src/utils.ts +++ b/packages/node/src/utils.ts @@ -97,6 +97,23 @@ export function getNodeResponseError(res: Stream.Writable | NodeHttpResponse): E return res.errored } +const NODE_RESPONSE_DISCONNECT_ERROR_CODES = new Set([ + 'ERR_HTTP2_STREAM_ERROR', + 'ERR_HTTP2_SESSION_ERROR', + 'ECONNRESET', + 'EPIPE', +]) + +/** + * Check whether a response error only means the client disconnected. + * + * A client disconnect only closes an http1 response, while on http2 it usually errors + * the response too, so these errors should be handled like a plain close. + */ +export function isNodeResponseDisconnectError(error: Error): boolean { + return NODE_RESPONSE_DISCONNECT_ERROR_CODES.has((error as { code?: unknown }).code) +} + /** * Destroy a body that won't be sent. `onError` is attached first, so an `error` * event from `destroy` is never unhandled.