fix(MockHttpSocket): handle response stream errors #548

kettanaito · 2024-04-12T20:01:01Z

When a mocked response stream receives an error (e.g. controller.error(error)), translate that to request errors (i.e. emit the "error" event and abort the request).

Todo

Check how Node.js handles these situations. Do they translate to the "error" event? To error responses?

kettanaito · 2024-04-12T20:25:25Z

Alright, so treating those response stream errors as request errors isn't right. Even the original comment says so.

The problem is, I cannot reproduce the error event on response when communicating with a Node.js server. I'm doing this:

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

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

  const request = http.get(httpServer.http.url('/resource'))
  request.on('error', requestErrorListener)
  request.on('response', (response) => {
    console.log('RESPONSE!')

    response.on('error', (error) => {
      console.log('ERROR!', error)
    })
  })

The response event is emitted but not the error event on the response.

@mikicho, is this roughly how you are reproducing this in a non-mocked scenario?

kettanaito · 2024-04-12T20:39:42Z

src/interceptors/ClientRequest/MockHttpSocket.ts

+          // This way, the client still receives the "response" event,
+          // and the actual stream error is forwarded as the "error"
+          // event on the http.IncomingMessage instance.
+          flushHeaders()


This will emit the response event on the client because it will receive the response headers.

kettanaito · 2024-04-12T20:40:39Z

src/interceptors/ClientRequest/MockHttpSocket.ts

+
+          // Forward response streaming errors as response errors.
+          /** @todo This doesn't quite do it. */
+          // this.destroy(error)


Needs help

I'm struggling to find a way to trigger this:

https://github.com/nodejs/node/blob/3a456c6db802b5b25594d3a9d235d4989e9f7829/lib/_http_incoming.js#L239

The error event on http.IncomingMessage is emitted when the socket is destroyed (implemented in the ._destroy() method). For some reason, calling this.destroy() on the HttpMockSocket doesn't trigger that.

kettanaito · 2024-04-12T20:41:48Z

src/interceptors/ClientRequest/MockHttpSocket.ts

-    if (httpHeaders.length > 0) {
-      flushHeaders()
-    }
+    flushHeaders()


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

…tream-error

kettanaito · 2024-04-16T16:12:25Z

How this works in non-mocked world

When there's a response stream error, the server throws an error and, usually, handles it as a 500 response:

response 500 Internal Server Error
RESPONSE DATA <!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Error</title>
</head>
<body>
<pre>Error: Cannot pipe, not readable<br>

This means that erroring the response stream just results in an error thrown. It doesn't get forwarded to ClientRequest or IncomingMessage in any special way, aside from a 500 response arriving to the client.

@mikicho, is this the same expectation Nock has in this scenario?

kettanaito · 2024-04-16T16:18:51Z

This should be fixed by #532 but this PR still contains viable tests and the fix (flushing response headers on unhandled response stream errors).

…tream-error

kettanaito · 2024-04-16T17:58:48Z

@mikicho, I think this is done! Response stream errors are now handled as 500 error responses. I believe that's what's happening in the actual server as well.

mikicho · 2024-04-16T18:25:25Z

src/interceptors/ClientRequest/MockHttpSocket.ts

        }
+      } catch (error) {
+        if (error instanceof Error) {


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

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.

mikicho · 2024-04-16T19:08:51Z

Is this the same expectation Nock has in this scenario?

No, Nock propagates the error event to the response (IncmingMessage).
I believe the scenario you described in here may not cover the entire picture because Node itself could emit an error for the underlying readable.

I think Nock's behavior provides better DX and is more predictable from the user's POV.

kettanaito · 2024-04-17T08:14:37Z

I think Nock's behavior provides better DX and is more predictable from the user's POV.

While Nock compatibility is the goal, we should focus on how Node.js behaves here, not Nock. From everything I've tried so far, Node doesn't handle response stream errors in any special way. Errors via controller.error(e) simply throw because by design, they meant to error if you try to operate with the stream afterward.

When piping a regular Readable, the source can destroy the stream, and that will propagate as the error event of IncomingMessage. That looks like a different use case though, and we should handle it as such. Perhaps the confusion here is caused by the fact that ReadableStream is not entirely the same behavior-wise as Readable, although from the source, it seems controller.error(e) should destroy Readable:

kettanaito · 2024-04-17T12:16:53Z

One more argument to preserving non-error exceptions is you can throw a Response instance and that will be used as the mocked response: https://github.com/mswjs/msw/blob/43a163b2aab87d124aecde05826729b896ad3484/test/node/rest-api/response/throw-response.node.test.ts#L26.

This is an intentional design to support Remix-like response throwing.

I've added this intention as automated tests in #553.

kettanaito · 2024-04-17T16:31:37Z

src/interceptors/ClientRequest/MockHttpSocket.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(createServerErrorResponse(error))


Treating all the response stream errors as 500 error responses! Consistency.

…tream-error

kettanaito · 2024-04-17T16:47:00Z

I will merge this one. If I missed anything, please let me know, we can iterate on it in the follow-up pull request. Thanks!

mikicho · 2024-04-17T17:23:37Z

While Nock compatibility is the goal, we should focus on how Node.js behaves here...

@kettanaito I don't think it is not a question of compatibility. The scenario, as I see it, is in-process Readable failure, not server-side failure, and in Node.js, probably extremely rare (I can't even think of such a case)

This is why I believe the current behavior is not quite right and could be causing some confusion. We take a user-side error and convert it to 500 error although the "server" was ok.
In other words, one can ask when the user would expect us to emit an error event for the response object:

http.get(..., res => { 
  res.on('error', () => {} 
}

kettanaito · 2024-04-17T17:42:51Z

We take a user-side error and convert it to 500 error although the "server" was ok.

But that's the point, it wasn't. When mocking, your request listener is the server. If there's an exception there, it's equivalent to the actual server getting an exception on its runtime, which is often handled as a 500 error response.

Note that this has no effect on the bypassed requests.

In other words, one can ask when the user would expect us to emit an error event for the response object:

Technically, when the response stream is destroyed. That's the only occasion per spec when IncomingMessage emits the error event. Calling controller.error(e), somehow, doesn't trigger the stream's destruction.

Here's the only scenario where I think the error will be emitted correctly:

http.get(url, (res) => {
  res.on('error', () => console.log('Error!'))
  res.destroy(new Error('Do not need this anymore'))
})

Destroying the IncomingMessage stream directly will emit the error event. But since you don't have access to the IncomingMessage in the request listener, you can't destroy the response that way.

mikicho · 2024-04-17T17:58:43Z

When mocking, your request listener is the server

While this is true, in this case, I see the error as coming from Node.js itself, not from the server. I think it would provide better DX.
Anyway, I think this is not a blocker, and we should release it and see what the users think.

kettanaito requested a review from mikicho April 12, 2024 20:01

kettanaito changed the base branch from main to feat/yet-another-socket-interceptor April 12, 2024 20:01

kettanaito mentioned this pull request Apr 12, 2024

feat(ClientRequest): use net.Socket interceptor #515

Merged

18 tasks

kettanaito added 2 commits April 12, 2024 14:02

fix(MockHttpSocket): handle response stream errors

a192a3a

test: ensure response/end are not emitted

3510477

kettanaito force-pushed the fix/forward-stream-error branch from 597d301 to 3510477 Compare April 12, 2024 20:03

fix: flush response headers

69679a7

kettanaito commented Apr 12, 2024

View reviewed changes

kettanaito added 2 commits April 15, 2024 23:30

Merge branch 'feat/yet-another-socket-interceptor' into fix/forward-s…

60f0f27

…tream-error

Merge branch 'feat/yet-another-socket-interceptor' into fix/forward-s…

a36b790

…tream-error

feat: handle response stream errors as 500 responses

8f84796

kettanaito added 5 commits April 16, 2024 19:21

Merge branch 'feat/yet-another-socket-interceptor' into fix/forward-s…

41a661f

…tream-error

fix(MockHttpSocket): translate unhandled errors to 500 responses

07834f1

fix: handle interceptor errors as 500 responses

84fca0e

test: fix miniflare test

22dedf5

fix: allow node-like errors

194c9c8

mikicho reviewed Apr 16, 2024

View reviewed changes

chore: merge with the existing test

e93cef2

kettanaito added 2 commits April 17, 2024 18:22

Merge branch 'main' into fix/forward-stream-error

9f04c0e

fix(MockHttpSocket): treat all response stream errors as 500 responses

48f3338

kettanaito commented Apr 17, 2024

View reviewed changes

Merge branch 'feat/yet-another-socket-interceptor' into fix/forward-s…

700212c

…tream-error

kettanaito merged commit 085d1ec into feat/yet-another-socket-interceptor Apr 17, 2024
2 checks passed

kettanaito deleted the fix/forward-stream-error branch April 17, 2024 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(MockHttpSocket): handle response stream errors #548

fix(MockHttpSocket): handle response stream errors #548

kettanaito commented Apr 12, 2024 •

edited

Loading

kettanaito commented Apr 12, 2024

kettanaito Apr 12, 2024

kettanaito Apr 12, 2024 •

edited

Loading

kettanaito Apr 12, 2024

kettanaito commented Apr 16, 2024

kettanaito commented Apr 16, 2024

kettanaito commented Apr 16, 2024

mikicho Apr 16, 2024 •

edited

Loading

kettanaito Apr 17, 2024

mikicho commented Apr 16, 2024 •

edited

Loading

kettanaito commented Apr 17, 2024

kettanaito commented Apr 17, 2024 •

edited

Loading

kettanaito Apr 17, 2024

kettanaito commented Apr 17, 2024

mikicho commented Apr 17, 2024

kettanaito commented Apr 17, 2024 •

edited

Loading

mikicho commented Apr 17, 2024

fix(MockHttpSocket): handle response stream errors #548

fix(MockHttpSocket): handle response stream errors #548

Conversation

kettanaito commented Apr 12, 2024 • edited Loading

Todo

kettanaito commented Apr 12, 2024

kettanaito Apr 12, 2024

Choose a reason for hiding this comment

kettanaito Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Needs help

kettanaito Apr 12, 2024

Choose a reason for hiding this comment

kettanaito commented Apr 16, 2024

How this works in non-mocked world

kettanaito commented Apr 16, 2024

kettanaito commented Apr 16, 2024

mikicho Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

kettanaito Apr 17, 2024

Choose a reason for hiding this comment

mikicho commented Apr 16, 2024 • edited Loading

kettanaito commented Apr 17, 2024

kettanaito commented Apr 17, 2024 • edited Loading

kettanaito Apr 17, 2024

Choose a reason for hiding this comment

kettanaito commented Apr 17, 2024

mikicho commented Apr 17, 2024

kettanaito commented Apr 17, 2024 • edited Loading

mikicho commented Apr 17, 2024

kettanaito commented Apr 12, 2024 •

edited

Loading

kettanaito Apr 12, 2024 •

edited

Loading

mikicho Apr 16, 2024 •

edited

Loading

mikicho commented Apr 16, 2024 •

edited

Loading

kettanaito commented Apr 17, 2024 •

edited

Loading

kettanaito commented Apr 17, 2024 •

edited

Loading