socket `connect` event is not emitted when sending multiple requests concurrently with a http agent `keepAlive: true`

mizozobu commented 2 months ago

Prerequisites

[X] I confirm my issue is not in the opened issues
[X] I confirm the Frequently Asked Questions didn't contain the answer to my issue

Environment check

[X] I'm using the latest msw version
[X] I'm using Node.js version 18 or higher

Node.js version

v22.7.0

Reproduction repository

https://github.com/mizozobu/msw-socket-mre

Snapshot of the repo for future reference. (I'll delete it once this issue gets resolved).

```js import http from 'node:http'; import { http as mswHttp, HttpResponse } from 'msw'; import { setupServer } from 'msw/node'; const handlers = [ mswHttp.get('http://exmaple.com/*', async({ request }) => { console.log('received', request.url); return HttpResponse.json({}); }), ]; const server = setupServer(...handlers); server.listen(); const agent = new http.Agent({ keepAlive: true, }); const sockets = new Set(); const sendRequest = async(id) => { console.log(`create request for id=${id}`); const res = await new Promise(resolve => { const req = http.request(`http://exmaple.com/${id}`, { agent }, (res) => { resolve(res.body); }); req.on('error', (e) => { console.error(`problem with request: ${e.message}`); }); req.once('socket', (socket) => { sockets.add(socket); console.log(sockets.size, 'unique sockets used'); if (socket.connecting) { socket.once('connect', () => { console.log(`socket for id=${id} connected`); req.end(); }); } else { req.end(); } }); }); return res; } const list = await Promise.all([ sendRequest('1'), sendRequest('2'), sendRequest('3'), ]); console.log(list); ```

Reproduction steps

npm i
node index.mjs

Current behavior

3 sockets are created, but only 1 socket emits connect event.

I encountered this when I was using msw with stripe. Some reproduction code was taken from the repo.

Expected behavior

All sockets emit connect event.

kettanaito commented 2 months ago

This is golden! Linking https://github.com/mswjs/interceptors/issues/594 for reference.

omairvaiyani commented 2 months ago

We encountered this issue in our test-suite with Stripe too

kettanaito commented 1 month ago

Discovery

This turned out to be a really interesting one!

It has nothing to do with reusing sockets or keepAlive. If you inspect what's happening, you will see that MSW doesn't intercept that request at all.

The root cause for this issue is that Interceptors require the request header to be written to the socket in order to dispatch the request event for the interceptor. The result of that event determines if the connection should be mocked or pass through. Only then the socket emits the connect event.

You can circumvent this by flushing request headers preemptively:

req.on('socket', (socket) => {
  socket.on('connect', () => req.end()
})
+req.flushHeaders()

If you do this, MSW will intercept the request, and emit the connect event on the socket the correct number of times. That already works as expected.

By default, Node.js buffers Socket writes before you call req.end(). That's precisely why MSW doesn't know about the request at all until you do. Calling req.flushHeaders() will cause Node.js to flush the request headers to the Socket, letting MSW know.

Alas, there is no way to listen when Node.js does its buffering (which happens here). I'd rather we kept the number of workarounds to the minimum.

What we should do, is somehow know if you've flushed the request headers or not (or called .end() or not). We can technically attach a _read implementation that will check the ClientRequest associated with the HTTP parser on the socket, and see if it has flushed its headers yet:

this._read = () => {
  const headerSent = !!this.parser?.outgoing._headerSent
  console.log('headers sent?', { headerSent })
}

This is less hacky but still not ideal. From what I can see, there is nothing else on the Socket instance that could indicate that (sockets can be used for different protocols, so them not having something related to a HTTP request makes sense). I was hoping maybe the writableState will be paused/corked/whatever, but that's not the case either. Node.js doesn't do writes at all, keeping headers in memory.

Node.js also starts the request stream processing if you write something to a request (req.write(chunk)). But your use case appears to be valid so we should support it.

kettanaito commented 1 month ago

This comes down to the Socket determining only the fact of a network connection (no requests sent yet), and the interceptor controlling the network on the request basis (so the request has to happen). There are no plans to give you any network-based APIs, those would be too low level and would only work in Node.js.

kettanaito commented 1 month ago

I've opened a fix at https://github.com/mswjs/interceptors/pull/643.

kettanaito commented 1 month ago

Solution

After testing this a bit and giving it some more thought, I think this use case is an inherent limitation of the interception algorithm we have:

In order to emit the connect event, it has to know if you've handled the request in any way (i.e. emit the request event on the interceptor);
In order to emit the request event, the request has to flush its headers so the parsing kicks in.

These two are contradictory.

You are locking the request header send by deferring req.end() to the connect event on the socket. This is possible with Node.js but you cannot use that with MSW. Unfortunately, there's no straightforward way we can address it either. Node.js provides us no means to tell apart a request that's still buffering. Besides, you can always write to a request after you create it but before you call .end() (and that's one of the reasons Node.js buffers the headers and makes them immutable once you flush them).

How to fix this?

You have two options to fix this problem.

Option 1: Call `req.end()` outside of `connect`

req.on('socket', (socket) => {
-  socket.on('connect', () => req.end())
+  req.end()
})

Option 2: Call `req.flushHeaders()` outside of `connect`

You can keep req.end() in the connect event listener, but then you have to flush the request headers explicitly to send them to the server, initiate the request, and also kick in the parsing on MSW's side.

req.on('socket', (socket) => {
  socket.on('connect', () => req.end())
})
+req.flushHeaders()

You can read more about .flushHeaders() in the Node.js docs.

This will be the recommendation for now. Contributions are welcome at https://github.com/mswjs/interceptors/pull/643, where I gave this bug a try, but arrived at an unstable fix.

jeyj0 commented 1 month ago

For those who also end up here due to the stripe SDK's requests not being intercepted:

Here's a potential workaround, which applies to you, if:

you have a fetch implementation available where you set up the stripe client, and
MSW can intercept requests made using that fetch implementation.

Workaround: tell stripe to use fetch instead!

const stripe = new Stripe(<STRIPE_SECRET_KEY>, {
  apiVersion: "<API_VERSION>",
  httpClient: Stripe.createFetchHttpClient(),
});

mswjs / msw