fix(ClientRequest): ignore request destroyed state when responding with a mocked response

[kettanaito]

• Fixes Responses with delay hang forever with "got" #426

[kettanaito]

Hey, @mikicho 👋 I've tried reproducing your issue from #426 in an integration test but it passes reliably. Do you spot any difference that might be causing this?

[mikicho]

You may want to try an address that does not exist like: nowhere.com
#426 (comment) (update 3)

it.only('supports timeout before resolving request as-is', async () => {
  interceptor.on('request', async ({request}) => {
    await sleep(750)
  })

  const requestStart = Date.now()
  const res = await got('http://test.example',{ retry: 0 })
  const requestEnd = Date.now()

  expect(res.statusCode).toBe(200)
  expect(res.body).toBe(`{"id":1}`)
  expect(requestEnd - requestStart).toBeGreaterThan(700)
})

[kettanaito]

I found it to be related to the sleep in the request listener. If I observe how the listeners get called (after #427), I can see that the library doesn't wait for the sleep to finish before calling the next listener. That's. . . odd.

Oh, one more thing: got has a built-in retry mechanism by default. It got me confused at first why I see 3 requests being made while there's just one got() statement. But if the request is handled correctly (e.g. delay is removed), then no retires happen, rightfully.

[mikicho]

Where it doesn't await?

Also, You can pass retry: 0 to got

[kettanaito]

Talking about this sleep:

https://github.com/mswjs/interceptors/pull/432/files#diff-d18cec8d1031bb154cb3fea3a0fefcf1c5ff3ee1f1db6c6346286e266832330bR46

You can pass retry: 0 to got

Good point. But we should support the default behavior with retries as well.

This is so unexpected I'm not ruling out code transformations by Vitest. We need to check if delay in request listeners functions correctly with other interceptors to narrow down the problematic area (it's likely the emitAsync function, although I can't put my finger on it):

interceptors/src/utils/emitAsync.ts

Lines 22 to 24 in 1b6b058

	for (const listener of listners) {
	await listener.apply(emitter, data)
	}

This is designed to call every listener sequentially, awaiting the previous to continue with the next. This way we can know when all the listeners have been called.

[kettanaito]

Root cause

I found the root cause for this. This line in strict-event-emitter implements once listeners by wrapping them in a higher function:

https://github.com/open-draft/strict-event-emitter/blob/b5da3bffe6b7913fe5ea27e3abe0285741765424/src/Emitter.ts#L81-L84

I've opened a fix in open-draft/strict-event-emitter#17

Because that function is not async, awaiting one-time listeners with await listener(...data) will resolve immediately, even if the wrapped listener is an async function with a sleep(). This is also the reason why this issue isn't reproducible with .on(), where the listener isn't wrapped and its reference is preserved.

[kettanaito]

Alas, the issue with got remains even with the fix to strict-event-emitter. There must be something else in play.

But, the sleep is now respected properly, as well as the sequential execution of the listeners. Now it is, indeed, something else.

[kettanaito]

Looking into got

When comparing the passing test (without delay) and a failing one, I've noticed one difference in how Got handles it. When there's any delay in the request listener, response.request.aborted in Got becomes true right here:

https://github.com/sindresorhus/got/blob/0da732f4650c398f3b2fea672f8916e6c7004c8f/source/as-promise/index.ts#L47-L50

This prevents Got from reading the response body initiated here:

https://github.com/sindresorhus/got/blob/0da732f4650c398f3b2fea672f8916e6c7004c8f/source/as-promise/index.ts#L55

And then forwarded to here and here. Without this response body reading, Got request pends forever despite receiving the response (the response event gets emitted correctly).

I'm trying to see what aborts the request in case of delay but can't find anything. I've added a setter on set aborted() {} in NodeClientRequest to no avail. I can confirm that Got uses the correctly patched http.get/http.request functions, which means patched NodeClientRequest. Perhaps the native Node.js logic sets aborted on the request?

There's also a number of .destroy() calls on the request in Got: during timeout, cancellation, etc. I will look into those and see if any of them get called.

[mikicho]

@kettanaito
aborted is a getter:
https://github.com/sindresorhus/got/blob/0da732f4650c398f3b2fea672f8916e6c7004c8f/source/core/index.ts#L2745

When debugging it, I saw that the this[kRequest]?.destroyed was true. Is there any chance we don't mock something related to the socket that causes the NodeRequest to fail? (more context)

Perhaps the native Node.js logic sets aborted on the request?

I think so

There's also a number of .destroy() calls on the request in Got: during timeout, cancellation, etc. I will look into those and see if any of them get called.

I looked in this direction, and it seems like got isn't the one who destroyed the request.

[kettanaito]

Root cause & The fix

@mikicho, yes! You've found it as well. I added a setter to destroyed (not aborted) on NodeClientRequest and saw what happens: when connecting to a non-existing host, the TLS class in Node.js destroys the request instance. While it's up to the request client to check that (like Got does), I've added a fail-safe logic to forcefully set this.destroyed = false only if in the mock response scenario (connection errors don't matter in that case).

The Got test is now passing.

[kettanaito]

@mikicho, would you have a minute to review these changes? I could use your eyes on this 🙏

[mikicho]

Instead of ignoring the failed request, can we handle the error by monkey-patching whatever is needed? This may not be urgent, and the issue may be resolved when/if we implement mocking at the socket level.
(#375)

[kettanaito]

I'm not sure what we could monkey-patch in this case though.

This assignment happens only for mocked responses. When you have a mock response returned, it shouldn't matter if there's been connection errors or not. This way we can keep the original connection logic from Node (handy for when you don't have mocked responses) but ignore any errors that occurred because they are irrelevant when providing a mock response.

Note that we still do the following:

1. Capture any errors occurring during the request, which includes connection errors;
2. If the consumer hasn't responded to the request, we replay those errors so they are never ignored when the request was performed as-is.

[kettanaito]

I think that Socket-based interception is largely not worth it (see #399). It's way too low-level and the benefits it provides aren't worth the cost (having to tap so deep you end up re-implementing Node buffer encoding methods since those aren't public).

[mikicho]

This assignment happens only for mocked responses...

I'm not an expert, but what if you want mock connection-related behavior, for example, Nock lets you mock delay in the connection and mock delay in the response.

Anyway, this is another feature or something anyway, so LGTM.

If the consumer hasn't responded to the request, we replay those errors so they are never ignored when the request was performed as-is.

Side question: Why can't we first check if there is a mocked response and only if negative, forward this to the real NodeClient?

[kettanaito]

Having more fine-grained delays would certainly be a feature worth exploring. For now, whichever delays you specify in the request listener are treated as response delays (they trigger after the connection has been made and the request has been written).

[mikicho]

Thanks!
We will probably need to to keep parity with what Nock have now if we want to move Nock to mswjs/interceptors

[kettanaito]

Let's raise this as a new feature request and discuss it in more detail.

[kettanaito]

Released: v0.25.3 🎉

This has been released in v0.25.3!

• 📄 Release notes
• 📦 npm package

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.

---

Predictable release automation by @ossjs/release.