search

mswjs / interceptors

Low-level network interception library.
https://npm.im/@mswjs/interceptors
MIT License
539 stars 123 forks source link

fix(MockHttpSocket): handle response stream errors #548

Closed kettanaito closed 5 months ago

kettanaito commented 5 months ago

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

kettanaito commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

kettanaito commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 https://github.com/mswjs/interceptors/pull/553.

kettanaito commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.