Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

response.complete is false in end listener #443

Closed
mikicho opened this issue Sep 22, 2023 · 17 comments · Fixed by #515
Closed

response.complete is false in end listener #443

mikicho opened this issue Sep 22, 2023 · 17 comments · Fixed by #515
Assignees

Comments

@mikicho
Copy link
Contributor

mikicho commented Sep 22, 2023

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
Copy link
Contributor Author

mikicho commented Sep 22, 2023

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

@mikicho mikicho closed this as completed Sep 22, 2023
@mikicho
Copy link
Contributor Author

mikicho commented Sep 22, 2023

@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?

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

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

@mikicho mikicho reopened this Sep 22, 2023
@kettanaito
Copy link
Member

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
Copy link
Contributor Author

mikicho commented Sep 22, 2023

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
Copy link
Member

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

@kettanaito
Copy link
Member

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:

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
Copy link
Member

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 kettanaito self-assigned this Sep 27, 2023
@kettanaito
Copy link
Member

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
Copy link
Contributor Author

mikicho commented Sep 28, 2023

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
Copy link
Member

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
Copy link

saurabh-asurion commented Sep 28, 2023

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
Copy link
Contributor Author

mikicho commented Sep 28, 2023

@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
Copy link
Member

@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
Copy link

@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
Copy link
Member

@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).

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
Copy link

saurabh-asurion commented Sep 29, 2023

@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 🙇

@kettanaito
Copy link
Member

Released: v0.32.0 🎉

This has been released in v0.32.0!

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.

BramKaashoek pushed a commit to labd/commercetools-node-mock that referenced this issue Oct 8, 2024
Removes the dependency `light-my-request` which was added in v2.2.0 to
circumvent an internal issue of msw
(mswjs/interceptors#443). This has since been
fixed and this should therefore no longer be necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants