interceptors icon indicating copy to clipboard operation
interceptors copied to clipboard

fix(MockHttpSocket): deadlock with clients that wait for `secureConnect` before writing anything

Open juicemia opened this issue 4 months ago • 1 comments

I opened https://github.com/mswjs/msw/issues/2569 because I noticed some behavior with an HTTP client that waits for the secureConnect event to happen before writing anything, which causes a deadlock because the MSW interceptor waits for write before firing secureConnect.

I've tried my best to also implement a solution given that I'm not an expert on how NodeJS sockets work by any means, but I wanted to at least try to be helpful. Any feedback on this PR is welcome. It seems to me like the code is a bit brittle. The implementation of the once method is the main thing that fixes that deadlock, but it caused other tests to fail so I went trying to handle each case one by one.

I also confirmed that it fixes the issue in my reproduction repository. I tested it by building the client on my Mac and linking it like this:

    "pnpm": {
      "overrides": {
      "@mswjs/interceptors": "file:/path/to/local/projects/juicemia/mswjs-interceptors"
    }

Then I removed my node_modules and re-ran pnpm i, and sure enough when running the tests none of them time out anymore.

One thing I would like is to add a test for this in this repo that reproduces the issue in the reproduction repo exactly. On my first iteration when I got the test in test/modules/http/regressions/http-socket-secure-connect-passthrough.test.ts working, all the tests in this repository were passing, but using the built package in my reproduction repository still didn't make it work. It was making that work that then caused all the regressions that I had to go fixing one by one. I'd love any tips on how I can make a test like that, I was thinking of copying how msw sets up ClientRequestInterceptor but it seemed like it would be very complicated to do.

juicemia avatar Aug 21 '25 20:08 juicemia

Hi, @juicemia. Appreciate your proposal and a fix here! I wonder how viable it is to proceed with MockHttpSocket given a total architecture overhaul at #741. We would be just adding more things to the plug that way.

The new architecture taps into net.* methods and then routes the sent packets through the interceptor-scoped parsers (think HTTP parser, SMTP parser, etc). That way, we aren't even working on the socket level anymore. All the socket events are emitted as-is for passthrough request. For mocked HTTPS/HTTP, we will just emit respective events, like so. That's similar to what we're doing now.

Todos

I should definitely take the test cases you added and add them to #741 before releasing it. Would be a nice way to confirm if those changes fix the issue.

kettanaito avatar Oct 19 '25 09:10 kettanaito