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(MockHttpSocket): handle response stream errors #548

Merged
Merged
Show file tree
Hide file tree
Changes from 11 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
57 changes: 38 additions & 19 deletions src/interceptors/ClientRequest/MockHttpSocket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,34 +248,53 @@ export class MockHttpSocket extends MockSocket {
}

if (response.body) {
const reader = response.body.getReader()

while (true) {
const { done, value } = await reader.read()

if (done) {
break
try {
const reader = response.body.getReader()

while (true) {
const { done, value } = await reader.read()

if (done) {
break
}

// Flush the headers upon the first chunk in the stream.
// This ensures the consumer will start receiving the response
// as it streams in (subsequent chunks are pushed).
if (httpHeaders.length > 0) {
flushHeaders(value)
continue
}

// Subsequent body chukns are push to the stream.
this.push(value)
}
} catch (error) {
if (error instanceof Error) {
Copy link
Contributor

@mikicho mikicho Apr 16, 2024

Choose a reason for hiding this comment

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

This is not a blocker, but I think we need to return 500 for any error.
The current implementation swallows the error from the user in case it does not extend Error

Copy link
Member Author

Choose a reason for hiding this comment

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

I completely agree! I revisited by previous point and found it wrong. I'm alining the uncaught exceptions handling in #555, and then will update this branch to migrate the implementation from NodeClientRequest.ts.

// Coerce response stream errors to 500 responses.
// Don't flush the original response headers because
// unhandled errors translate to 500 error responses forcefully.
this.respondWith(
new Response(
JSON.stringify({
name: error.name,
message: error.message,
stack: error.stack,
}),
{ status: 500, statusText: 'Unhandled Exception' }
)
)

// Flush the headers upon the first chunk in the stream.
// This ensures the consumer will start receiving the response
// as it streams in (subsequent chunks are pushed).
if (httpHeaders.length > 0) {
flushHeaders(value)
continue
return
}

// Subsequent body chukns are push to the stream.
this.push(value)
return
}
}

// If the headers were not flushed up to this point,
// this means the response either had no body or had
// an empty body stream. Flush the headers.
if (httpHeaders.length > 0) {
flushHeaders()
}
flushHeaders()
Copy link
Member Author

Choose a reason for hiding this comment

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

It's safe to call flushHeaders() because it will do nothing if the headers have already been flushed.


// Close the socket if the connection wasn't marked as keep-alive.
if (!this.shouldKeepAlive) {
Expand Down
28 changes: 27 additions & 1 deletion src/interceptors/ClientRequest/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { MockAgent, MockHttpsAgent } from './agents'
import { emitAsync } from '../../utils/emitAsync'
import { toInteractiveRequest } from '../../utils/toInteractiveRequest'
import { normalizeClientRequestArgs } from './utils/normalizeClientRequestArgs'
import { isNodeLikeError } from '../../utils/isNodeLikeError'

export class ClientRequestInterceptor extends Interceptor<HttpRequestEventMap> {
static symbol = Symbol('client-request-interceptor')
Expand Down Expand Up @@ -144,13 +145,38 @@ export class ClientRequestInterceptor extends Interceptor<HttpRequestEventMap> {
})

if (listenerResult.error) {
socket.errorWith(listenerResult.error)
// Allow mocking Node-like errors.
if (isNodeLikeError(listenerResult.error)) {
socket.errorWith(listenerResult.error)
return
}

socket.respondWith(
new Response(
JSON.stringify({
name: listenerResult.error.name,
message: listenerResult.error.message,
stack: listenerResult.error.stack,
}),
{
status: 500,
statusText: 'Unhandled Exception',
headers: {
'Content-Type': 'application/json',
},
}
)
)
return
}

const mockedResponse = listenerResult.data

if (mockedResponse) {
/**
* @note The `.respondWith()` method will handle "Response.error()".
* Maybe we should make all interceptors do that?
*/
socket.respondWith(mockedResponse)
return
}
Expand Down
1 change: 0 additions & 1 deletion test/modules/http/compliance/http-req-write.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,6 @@ it('calls all write callbacks before the mocked response', async () => {
method: 'POST',
})
request.write('one', () => {
console.log('write callback!')
request.end()
})

Expand Down
88 changes: 88 additions & 0 deletions test/modules/http/compliance/http-res-readable.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/**
* @vitest-environment node
*/
import { vi, it, expect, beforeAll, afterAll } from 'vitest'
import http from 'node:http'
import { Readable } from 'node:stream'
import { HttpServer } from '@open-draft/test-server/http'
import { ClientRequestInterceptor } from '../../../../src/interceptors/ClientRequest'
import { waitForClientRequest } from '../../../helpers'

function createErrorStream() {
return new ReadableStream({
start(controller) {
controller.enqueue(new TextEncoder().encode('original'))
queueMicrotask(() => {
controller.error(new Error('stream error'))
})
},
})
}

const httpServer = new HttpServer((app) => {
app.get('/resource', (req, res) => {
res.pipe(Readable.fromWeb(createErrorStream()))
})
})

const interceptor = new ClientRequestInterceptor()

beforeAll(async () => {
interceptor.apply()
await httpServer.listen()
})

afterAll(async () => {
interceptor.dispose()
await httpServer.close()
})

it('supports ReadableStream as a mocked response', async () => {
const encoder = new TextEncoder()
interceptor.once('request', ({ request }) => {
const stream = new ReadableStream({
start(controller) {
controller.enqueue(encoder.encode('hello'))
controller.enqueue(encoder.encode(' '))
controller.enqueue(encoder.encode('world'))
controller.close()
},
})
request.respondWith(new Response(stream))
})

const request = http.get('http://example.com/resource')
const { text } = await waitForClientRequest(request)
expect(await text()).toBe('hello world')
})

it('forwards ReadableStream errors to the request', async () => {
const requestErrorListener = vi.fn()
const responseErrorListener = vi.fn()

interceptor.once('request', ({ request }) => {
request.respondWith(new Response(createErrorStream()))
})

const request = http.get(httpServer.http.url('/resource'))
request.on('error', requestErrorListener)
request.on('response', (response) => {
response.on('error', responseErrorListener)
})

const response = await vi.waitFor(() => {
return new Promise<http.IncomingMessage>((resolve) => {
request.on('response', resolve)
})
})

// Response stream errors are translated to unhandled exceptions,
// and then the server decides how to handle them. This is often
// done as returning a 500 response.
expect(response.statusCode).toBe(500)
expect(response.statusMessage).toBe('Unhandled Exception')

// Response stream errors are not request errors.
expect(requestErrorListener).not.toHaveBeenCalled()
expect(request.destroyed).toBe(false)
})
24 changes: 14 additions & 10 deletions test/third-party/miniflare.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,12 @@ const interceptor = new BatchInterceptor({
],
})

const requestListener = vi.fn().mockImplementation(({ request }) => {
request.respondWith(new Response('mocked-body'))
})

interceptor.on('request', requestListener)

beforeAll(() => {
interceptor.apply()
})

afterEach(() => {
interceptor.removeAllListeners()
vi.clearAllMocks()
})

Expand All @@ -34,26 +29,35 @@ afterAll(() => {
})

test('responds to fetch', async () => {
interceptor.once('request', ({ request }) => {
request.respondWith(new Response('mocked-body'))
})

const response = await fetch('https://example.com')
expect(response.status).toEqual(200)
expect(await response.text()).toEqual('mocked-body')
expect(requestListener).toHaveBeenCalledTimes(1)
})

test('responds to http.get', async () => {
interceptor.once('request', ({ request }) => {
request.respondWith(new Response('mocked-body'))
})

const { resBody } = await httpGet('http://example.com')
expect(resBody).toEqual('mocked-body')
expect(requestListener).toHaveBeenCalledTimes(1)
})

test('responds to https.get', async () => {
interceptor.once('request', ({ request }) => {
request.respondWith(new Response('mocked-body'))
})

const { resBody } = await httpsGet('https://example.com')
expect(resBody).toEqual('mocked-body')
expect(requestListener).toHaveBeenCalledTimes(1)
})

test('throws when responding with a network error', async () => {
requestListener.mockImplementationOnce(({ request }) => {
interceptor.once('request', ({ request }) => {
/**
* @note "Response.error()" static method is NOT implemented in Miniflare.
* This expression will throw.
Expand Down
Loading