Fix: High unix socket concurrency #761
Conversation
| * the mock TLS properties (set in constructor) to remain accessible | ||
| * until real values are available. | ||
| */ | ||
| if (Reflect.get(socket, 'encrypted')) { |
There was a problem hiding this comment.
In this context, socket.encrypted is guaranteed, right? We might as well skip this if entirely or make an invariant to ensure it instead.
| this.emit('connect') | ||
| }) | ||
| .on('secureConnect', () => this.emit('secureConnect')) | ||
| .on('secureConnect', () => { |
There was a problem hiding this comment.
Do I remember it right that listening to the request.on('socket') won't give you any TLS properties until secureConnect is emitted on the socket? Is that the correct way to access those properties (like socket.encrypted)?
There was a problem hiding this comment.
MockHttpSocket provides mock TLS values in its constructor so they're available immediately. For passthrough, mock values remain accessible until they're replaced with real values on secureConnect
Regarding the use of Reflect.get, my understanding is that it's been used to avoid TypeScript issues. createConnection() returns net.Socket (which doesn't have an encrypted), but at runtime this is actually a TLSSocket. We could potentially cast to TLSSocket here and access it as socket.encrypted? It's a slightly more type-safe approach
|
|
||
| const HTTP_SOCKET_PATH = path.join(__dirname, './test-early-return.sock') | ||
|
|
||
| const httpServer = http.createServer((req, res) => { |
There was a problem hiding this comment.
We should have @open-draft/test-server installed already. Should use that one.
There was a problem hiding this comment.
Is @open-draft/test-server compatible with UNIX sockets? I followed the pattern from http-unix-socket.test.ts here
| const { res, text } = await waitForClientRequest(request) | ||
|
|
||
| expect(res.statusCode).toBe(200) | ||
| expect(await text()).toBe('ok') |
There was a problem hiding this comment.
Prefer await expect(text()).resolves.toBe('ok'). Here's why.
kettanaito
left a comment
kettanaito
left a comment
There was a problem hiding this comment.
Hi, @georgewaters. Thank you for working on this, looks like a great change. I left a few minor comments here and there, could you please take a look and let me know if something doesn't make sense?
Hi @kettanaito, happy new year! Changes addressed and comments replied to. One that might need further discussion is around accessing the TLS properties on |
2 participants
Addresses #760
Problem
When no request listeners are attached to the interceptor,
handleRequest()goes through unnecessary async machinery before calling passthrough(). These async operations create microtask checkpoints that allow requests to pile up, and when they all passthrough simultaneously, socket contention causes EPIPE errors.This particularly affects testcontainers usage where MSW is active but not intercepting Docker API requests.
Solution
Add an early return in
handleRequestwhen no listeners are attached:Additionally, moved TLS property forwarding in MockHttpSocket to the
secureConnectevent to ensure mock TLS properties remain accessible until real values are available.Reproduction
The issue is intermittent and timing-dependent, making it difficult to reproduce reliably in automated tests. A minimal reproduction is available at: https://github.com/georgewaters/testcontainers-kafka-msw-repro
Verification
Tested against Docker's Unix socket with 10 runs × 2000 concurrent requests each: