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

[kettanaito]

• Fixes #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 https://github.com/mswjs/interceptors/issues/426#issuecomment-1725813242 (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):

https://github.com/mswjs/interceptors/blob/1b6b05875c801d3ef9f504a7a935128c0590c438/src/utils/emitAsync.ts#L22-L24

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 https://github.com/open-draft/strict-event-emitter/pull/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 🙏

[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.