We can spy on ClientRequest.prototype.write() and ClientRequest.prototype.end() to set an internal [kRequestStream] readable stream. That way, we can access the stream before the socket emits the connect event.

Since the body stream is the last thing we need to call the request listener, we can call the listener even before deciding whether to emit the connect+ready on the socket instance.

@mikicho, what do you think about this approach?

Yet another attempt at the Socket-based interceptor

The N time's a charm. :)

Since the body stream is the last thing we need to call the request listener, we can call the listener even before deciding whether to emit the connect+ready on the socket instance.

I like the direction you are going!

Would it work in the following case?

const req = http.request(...)
req.write('sync') // OK

await sleep(1000)
req.write('async') // I think it will work, but I'm not sure about it. my concern is that the ClientRequest would await for the `connect` event to continue
req.end()

It appears that the process.binding function is going to be removed.
Update: seems like we can use const { HTTPParser } = require('_http_common') (or import { HTTPParser } from 'node:_http_common') instead.

Also, a question, Would the socket-based interceptor support fetch as well? or it would still require a different interceptor?
If not, we may implement very similar apparch with setGlobalDispatcher

Request event duplication issue

With this approach, we have to mock the connection to the host every time in order for the ClientRequest (the higher-level consumer) to start streaming the buffered request body through the socket (it won't start that until it receives the connect event).

This means a few different sequences of events depending on the type of the request.

Request to an existing endpoint with a mocked response

1. lookup (mocked).
2. connect (mocked).
3. ready (mocked).
4. INTERNAL.
   1. Reading the request body.
   2. Firing "request" on the intercepted.
   3. Piping the mocked response to the socket.
5. data.
6. response.
7. end.

Request to a non-existing endpoint with a mocked response

The same thing as the one above.

Request to a non-existing host without a mocked response

This is where things get a bit tricky.

1. lookup (mocked).
2. connect (mocked).
3. ready (mocked).
4. INTERNAL (the same request streaming to the interceptor).
5. error. Emit the suppressed lookup (connection) error since there's no mock in the interceptor so the consumer must receive the error about requesting a non-existing host.

This scenario is different from the event order in the unpatched scenario. The initial lookup event would include an error, and mirror itself in the error event on the socket also.

In our case, the lookup/connect/ready are successful. This is problematic for a few reasons:

1. Diverges from the "normal" event flow.
2. The socket errored and when the client will try writing to it (request body), while we will be able to forward those chunks to the HTTPParser, the underlying socket will throw Error: write EBADF error, which I believe indicates an invalid write.

While no direct consumers care about the lookup/connect/ready events, the error event is rather crucial. In either case, keeping the event flow as close to the normal behavior as possible is desirable.

Ideas how to resolve this

Option 1: Shadow request

We can spy on the net.Socket, and whenever it's constructed, construct a "shadow" copy of it internally and run the entire request/response on that shadow while keeping the original socket pending.

class SocketController {
    constructor(socket) {
        this.socket = socket;
        // Create a shadow socket.
        this.shadow = new net.Socket(...socketArgs);
        
        // Always mock the connection on the shadow.
        this.mockConnect(this.shadow)
        
        // Observe the shadow, not the original socket.
        this.shadow.write = proxy
        this.shadow.push = proxy
        
        this.onShadowHasMock = (response) => {
            // If the shadow socket received a mocked response
            // from the "request" listener in the interceptor,
            // mock the original socket connection (it doesn't matter now)
            // and pipe the response to the original socket.
            this.mockConnect(this.socket)
            pipeResponse(response, this.socket)
        }
        
        this.onShadowHasNoMock = () => {
            // If the interceptor had no mocked response for this request,
            // continue with the original socket as-is.
            // The original socket has been pending up to this moment.
        }
        
    }
}

In order for us to still respect the original socket and not to implement a dummy Socket class to provision this "waiting" period, we can do something like an infinitely pending promise on the crucial methods of the original socket that we then resolve in case of onShadowHasNoMock.

this.socket.emit = applyProxyHere((...args) => {
    waitForShadow().then(() => {
        Reflect.apply(...args)
    })
})

If the shadow socket has a mock, the waitForShadow promise will keep pending and won't ever be resolved (alternatively, we can either resolve it with a flag or reject it and provide a .catch() handler). If the shadow socket finds no mocked response, it will resolve the promise, which will trigger the method on the original socket in the order that they were originally called.

This should include at least .emit and .write, but may require to add the promise plug to more methods.

Problems with this solution:

• The shadow socket won't receive any writes until the original socket pipes them to the shadow socket. And the original socket won't start piping unless the ClientRequest receives the connect event from it. Catch 22.

I tried solving the socket event order issue in f7fded8. I believe it works great, now I'm writing tests to confirm that.

this.mockConnect(this.shadow)

How is this propogate the connect/ready/lookup events to the ClientRequest so it will continue to write the data?

Catch 22

Problems with this solution:

Probably not a problem, but I want to make sure I understand, in this option we will emit connect/ready/lookup twice in case of non-mocked request, right?

While no direct consumers care about the lookup/connect/ready events

It seems like Axios doesn't use this, but got exposes this data to its users, I'm not sure for what tho...

I opened a feature request for official mocking API like undici has: nodejs/node#51979

It seems like Axios doesn't use this, but got exposes this data to its users, I'm not sure for what tho...

It doesn't expose that data but rather hooks into those socket events to provide timings measurements. That's okay. If you take a look at the current state of the branch, it emits the events correctly for all scenarios we've talked about. There's no duplication.

I opened a feature request for official mocking API like undici has: nodejs/node#51979

This is great! I'd certainly love to hear Node's opinion on this. I would count on this landing any time soon so our work on the Socket interceptor is relevant for at least a few years.

Probably not a problem, but I want to make sure I understand, in this option we will emit connect/ready/lookup twice in case of non-mocked request, right?

Not anymore.

Not anymore.

This is impressive! how so?

Need help

I've pushed the latest state of the changes that supports both HTTP and HTTPS requests. I've moved things around a bit, mainly made the MockSocket to be agnostic of the type of the messages sent (just giving us the observability), introduced a MockHttpSocket and utilizes the mock socket and pipes its streams through the HTTP parser, and then integrates with the new _ClientRequestInterceptor that's now Agent-based (we aren't mocking the ClientRequest at all now).

I'm encountering errors when intercepting HTTP/HTTPS requests with a body. Error:

pnpm test:node test/modules/http/intercept/http.request.test.ts

 FAIL  modules/http/intercept/http.request.test.ts > intercepts a POST request
AbortError: The operation was aborted
 ❯ Readable.emit ../node:events:513:28

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { code: 'ABORT_ERR' }
Caused by: Error: Premature close
 ❯ Readable.emit ../node:events:513:28

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { code: 'ERR_STREAM_PREMATURE_CLOSE' }
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

This indicates either that the MockSocket connection has been closed (not the case, the connection is never established to begin with), or that the MockSocket is still in writable state when we try to .emit('data') when forwarding data events from the original socket (in the passthrough() method). The latter seems to be more likely since this.writable is still true. You can also observe the finish/end events on the mock socket to see that they are emitted after the passthrough() start.

This makes sense because right now we call socket.passthrough() in the interceptor on the request event, which fires as soon as the request headers are sent. There may be some time passing until the request body has been written completely. I think this indicates an issue that we mustn't start the passthrough until the socket is finished or is in the readable state.

I've tried things like

// the interceptor
socket.once('end', () => socket.passthrough())

But they haven't fixed the problem.

Isolated example

I've put up an isolated example of this in https://github.com/kettanaito/node-tls-mock. Strangely, all tests are passing there, even for requests with body. The socket implementation seems to be the same. I advise to inspect the differences first to see if I've missed something while porting it to this branch.

• The passing test with body
• The MockSocket implementation

Fixing the failing tests in #548.

Opened a fix to forward TLS Socket properties (authorized, encrypted, etc.): #556

Maybe we want to merge this to fix IPv6?

Nock (unintentionally?) covers an edge case of a request that ends in the connect event.
With current socket-based implementation the request never ends.

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())
});


const req = http.request('http://example.com')

req.on('socket', socket => {
  socket.once('connect', () => {
    console.log(1);  // never get called when innterceptor applied.
    req.end()
  })
  socket.once('secureConnect', console.log)
})

WDYT?

How do we need to handle Expect: 100-continue request header?

What Node.js does:

1. flush headers (docs, source)
2. Server side (should) returns 100 Continue response (docs)
3. Node emits continue event. (docs)

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())
});
const req = http.request({
  host: 'example.com',
  path: '/',
  headers: { Expect: '100-continue' },
})
req.on('response', res => {
  res.on('data', console.log)
  res.on('end', () => { })
})
req.on('continue', () => {
  console.log(1); // never get called when interceptor applied.
  req.end()
})

I wonder if we should emit a continue event before calling the interceptor.
I think I can open PR when we decide how to handle this.

Nock (unintentionally?) covers an edge case of a request that ends in the connect event.

@mikicho, we cannot cover this. connect depends on req.end() so it cannot be called in the connect event listener when the interceptor is enabled. Are there any other details around that test case in Nock (is it a regression)?

How do we need to handle Expect: 100-continue request header?

This reads to be an http.ClientRequest level of behavior. Is it not happening already? That's odd.

We should support it but we should also be careful not to prematurely support it. As in, if it shouldn't belong to the Socket level and we ignore a potential bug that prevents Node.js from calling checkContinue() as it does in a non-interceptor scenario.

Let's add an integration test for this first and then see! Would love to dive deeper to learn what exactly Node.js does step-by-step, and at which step it stops when using the interceptor.

@mikicho, hi! You've mentioned there are also some remaining issues left here. Could you please post them in the comment to this pull request? I'd love to see what's left so I can tackle some of them whenever I have a moment. Thank you!