mswjs / interceptors

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

Support http.request flushHeaders #439

Closed mikicho closed 2 months ago

mikicho commented 11 months ago

flushHeaders should kickstart the request (same as end): https://nodejs.org/api/http.html#requestflushheaders Nock test: https://github.com/nock/nock/blob/main/tests/test_intercept.js#L964

kettanaito commented 11 months ago

That's interesting, I didn't know you could trigger the request by .flushHeaders(). I assume this is relevant only for requests without body? I need to read more about this, see some usage examples before I'm certain how to correctly address this in the interceptor.

As of now, I'm wondering if .flushHeaders() is the equivalent of .end().

kettanaito commented 11 months ago

A bit of digging around.

mikicho commented 11 months ago

A bit more context from the docs:

For efficiency reasons, Node.js normally buffers the request headers until request.end() is called or the first chunk of request data is written. It then tries to pack the request headers and data into a single TCP packet. That's usually desired (it saves a TCP round-trip), but not when the first data is not sent until possibly much later. request.flushHeaders() bypasses the optimization and kickstarts the request

kettanaito commented 11 months ago

I think I understand how flushHeaders() is meant to work. Reading through the mentioned test cases from Node.js helped a lot.

This considered, implementing a full support for .flushHeaders() is a bit problematic in the current state of Interceptors. This is related to the discussion in https://github.com/mswjs/interceptors/issues/400#issuecomment-1677689790. Shortly, right now the library constructs the Request instance and emits the request event after the request has been completely written and sent.

With .flushHeaders(), it'd have to revert its event flow a little, and do this:

  1. Construct a ReadableStream internally and write/end that stream based on the .write()/.end() of the underlying ClientRequest.
  2. Construct a Request instance as soon as the request is constructed (or its headers are flushed, not sure what's more semantically correct here). This way the consumer would get the request listener called immediately but the request instance will have its body stream possibly in the open/writing state at that time.

As I shared in the linked comment, this is possible but will require some internal refactoring to achieve.

kettanaito commented 11 months ago

@mikicho, do you happen to know how Nock supports .flushHeaders()?

mikicho commented 11 months ago

Thanks for the info! Very insightful! From my POV, this is insignificant; we can leave this open and tackle it later.

do you happen to know how Nock supports .flushHeaders()?

I didn't dig into it, but it seems like it starts the playback, which I THINK eventually emits an end event.

https://github.com/nock/nock/blob/feaa66fa64d24f95937ef759cdd5a7ca07646f1c/lib/intercepted_request_router.js#L221

kettanaito commented 11 months ago

I think you're right. Nock seems to check if the socket connection has been established, if it hasn't, it sets an internal flag to start the playback as soon as it does. If it has, it just starts the playback, which eventually calls newReq.end():

https://github.com/nock/nock/blob/feaa66fa64d24f95937ef759cdd5a7ca07646f1c/lib/intercepted_request_router.js#L343

I wonder how would Nock handled scenarios such as writing to request body after req.flushHeaders()? If it calls .end() and then the consumer tries to write to the request (which is a legal operation), Node will throw "cannot write after end".

I believe it's rather a big semantic difference between:

  1. Emitting "request" when the request has been sent;
  2. Emitting "request" when the request has started/been declared.

I want to lean toward the former, to be honest. But in the context of .flushHeader(), this distinction remains relevant: the request is sent with that method, its body just can be written afterward. So we can still support this method by implementing it a bit differently than .end() and keeping track of whether the request body has finished writing (the .end() has been called).

kettanaito commented 11 months ago

I think we can implement .flushHeaders() without any substantial refactoring by doing this:

Step 1: Emit request event on .flushHeaders()

Since this effectively means the request is sent and ready to be received by the server, emit the request event on the Interceptor to let the consumer know.

Step 2: Prepare a request body stream

This is the minimal amount of refactoring we have to do. We need to represent the request body in an internal ReadableStream (most likely will use just Readable since it's easier to write to it) and then provide that stream directly on the created Request instance during .flushHeaders().

This way the consumer will react to the request but if they decide to read its body, they'd have to wait for that request body stream to be populated, if ever.

Step 3: Adjust the .end() call

Since in the case of .flushHeaders() we have already emitted the request event, we need to keep some internal flag to indicate that and do not emit that event in the .end() method if it has already been emitted before. In the .flushHeaders() + .end() combination, the .end() call will only serve as the end of the request body stream, letting the consumer know that the last request body chunk has been written.

if (!this.requestSent) {
  // Emit "request"
}

We'd also want to push the entire emitAsync and mock response logic to .flushHeaders() (or reuse it) since .end() is not guaranteed to be called at all. This may be a bit tricky but I think the best approach here is to abstract the asyncEmit + until(mockedResponse) logic from .end() and reuse it across the two methods (.end() and .flushHeaders()).

kettanaito commented 2 months ago

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.