response.complete is false in end listener

[mikicho]

We set the this.response.complete after the response.push(null), which emits the end event.
Therefore, I think the response.complete is always false in the end event.

IIUC the docs, it should be true:
https://nodejs.org/api/http.html#messagecomplete

I think we should switch these lines: https://github.com/mswjs/interceptors/blob/main/src/interceptors/ClientRequest/NodeClientRequest.ts#L489-L490

[mikicho]

Maybe I'm wrong. I'll reopen if needed

[mikicho]

@kettanaito

1. I guess I'm doing something wrong, but this script doesn't print true/false:

const { ClientRequestInterceptor } = require('@mswjs/interceptors/ClientRequest')
const http = require('http')

const interceptor = new ClientRequestInterceptor({
  name: 'my-interceptor',
})
interceptor.apply();
interceptor.on('request', ({request}) => {
  request.respondWith(new Response('hey', { status: 200 }))
});

http.get('http://example.test', res => {
  res.on('end', () => {
    console.log(res.complete)
  })
})

Do you spot my error here?

2. I didn't have time to find the root cause now, but I think the complete property is always false because we clone the response: https://github.com/mswjs/interceptors/blob/main/src/interceptors/ClientRequest/NodeClientRequest.ts#L330-L332

3. I'll try to reproduce it with a test later

[kettanaito]

Do you spot my error here?

Afaik, you need to read the response body if you want for it to emit the end event. Simply add the res.on('data', () => {}) listener and the end will be emitted as you expect.

[mikicho]

thanks!

const { ClientRequestInterceptor } = require('@mswjs/interceptors/ClientRequest')
const http = require('http')

const interceptor = new ClientRequestInterceptor({
  name: 'my-interceptor',
})
interceptor.apply();
interceptor.on('request', ({request}) => {
  request.respondWith(new Response('hey', { status: 200 }))
});

http.get('http://example.test', res => {
  res.on('data', () => {})
  res.on('end', () => {
    console.log(res.complete) // prints false
  })
})

res.complete prints false. Is this expected?

[kettanaito]

I think you're right, res.complete should be true by the time the end event is emitted. Will open a fix.

[kettanaito]

Right now, when using Interceptors, the res.complete property always remains false. That's due to us cloning the original emitted response instance right here:

interceptors/src/interceptors/ClientRequest/NodeClientRequest.ts

Lines 330 to 332 in 01aa28d

	const response = data[0] as IncomingMessage
	const firstClone = cloneIncomingMessage(response)
	const secondClone = cloneIncomingMessage(response)

We do this cloning so then the received response can be read multiple times:

1. Internally by Interceptors to construct a Response instance out of it.
2. Externally by the developer via data and end events.

And while both use cases are working right now, it seems that something is off in regards to the complete and maybe even the end event.

[kettanaito]

In Node.js, it's the stream module that sets the complete property to true when the message is complete:

https://github.com/nodejs/node/blob/67546529c6e8181aa3f93aca5cbbada29bfd29e9/lib/_http_common.js#L142

https://github.com/nodejs/node/blob/67546529c6e8181aa3f93aca5cbbada29bfd29e9/lib/_http_common.js#L168

Perhaps, our cloneIncomingMessage doesn't propagate that [kOnMessageComplete] symbol to the cloned messages.

[kettanaito]

I think my initial implementation of IncomingMessage cloning was a bit too radical and mainly based on my lack of knowledge of streams in Node.js. I can't say it's much better now, but I think what we should do instead is to pipe the original response through a PipeThrough() stream that would allow us to react to incoming chunks, and then pipe those chunks to a Readable that we'd pass to the createResponse().

I think if we pipe it to a Readable directly, it would still error if we try to read them twice but I can verify.

[mikicho]

From a quick look, I think this is what nock does: https://github.com/nock/nock/blob/main/lib/playback_interceptor.js#L275-L291

[kettanaito]

Thanks for the reference! In Interceptors, we consume the same response body multiple times so we have to have some mechanism in place not to block further consumers if we read the response body internally (for example, in order to construct a Response representation out of IncomingMessage). Nothing in the linked code would account for that, as it's more about converting the body to a stream and reading it than allowing it to read the same stream multiple times. I think that's due to the API difference between the libraries.

[saurabh-asurion]

hey @kettanaito & @mikicho, I am running into the same issue (i.e. response.complete is false) even when using mswjs/interceptors to just observe the response. I can confirm that I am not mocking the response in interceptor.('request', ({.....}))

Is there a workaround I can go with in the short term? Thanks in advance for the help!

[mikicho]

@saurabh-asurion
Move to Nock 😉 (Just kidding, we are working hard to replace the Nock underlining interception engine with @mswjs/interceptors)

I don't think there is a workaround, but I have a feeling we will solve it soon :) 🤞

[kettanaito]

@saurabh-asurion, thanks for reporting you're experiencing this issue as well. I need to think on what's the expected behavior here. The response event is emitted as soon as we received the response, which doesn't necessarily mean that we've received its entire body (for which complete stands). But this is largely irrelevant for you as the consumer.

I'm working on the fix for this and would like to have it merged sometime on Monday. Thanks for patience.

[saurabh-asurion]

@kettanaito, really appreciate you explaining the issue. I spent quite some time looking into the interceptors codebase but wasn't able to figure out the root cause. Thank you for working on a fix!

[kettanaito]

@saurabh-asurion, if you'd like to have more context, we never set complete to true right now due to the way we clone the original response (IncomingMessage).

interceptors/src/interceptors/ClientRequest/NodeClientRequest.ts

Lines 330 to 332 in 01aa28d

	const response = data[0] as IncomingMessage
	const firstClone = cloneIncomingMessage(response)
	const secondClone = cloneIncomingMessage(response)

Because it's not the IncomingMessage itself that sets its complete property. In fact, that property doesn't even belong to that class, it's a property from the generic stream class and it's being handled there by Node.js. But due to the way we're cloning the message, it never ends up inhering from that stream class (for one reason or the other).

The actual issue here is that we don't have to clone it that aggressively, to begin with. I think we should be fine with passing it through and observing the response's data (and don't meddling with the super.emit() call so it'd emit the actual response instance).

[saurabh-asurion]

@kettanaito, after going down a (very informative) rabbit hole of IncomingMessage, stream-readable, and attempting to consume and clone the stream (not IncomingMessage) I was arriving at the same conclusion. I really appreciate the pointers from you (and @mikicho in this thread) as it's helping me in trying out different approaches and learning along the way. I doubt I will be able to solve it on my own but I will definitely try:)

ps: this is the only library (that I have come across) that allows me to intercept the request/response without having to write separate interceptors for each HTTP client library 🙇