Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support cross-process interception via setupRemoteServer #1617

Draft
wants to merge 237 commits into
base: main
Choose a base branch
from

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented May 12, 2023

This is an experimental feature. It's unlikely to ship before 2.0.

Intention

Introduce an API that allows one process to modify the traffic of another process. The most apparent application for this is testing server-side behaviors of a JavaScript application:

// app.js
forwardNetworkToRemote()

export const loader = async () => {
  const res = await fetch('https://example.com/resource')
}
// app.test.js
it('fetches the user server-side', async () => {
  let a = listenToRemoteNetwork(targetProcess)
  modifyNetwork(a)
  // ...render
  // ...assert
})

This is an example API. For the exact proposed API, keep reading.

This API is designed exclusively for use cases when the request-issuing process and the request-resolving process (i.e. where you run MSW) are two different processes.

Proposed API

With consideration to the existing MSW user experience, I suggest we add a setupRemoteServer() API that implements the SetupApi interface and has a similar API to setupServer. The main user-facing distinction here is that setupRemoteServer is affecting a remote process, as indicated by the name.

import { http } from 'msw'
import { setupRemoteServer } from 'msw/node'

const remote = setupRemoteServer(...initialHandlers)

// Notice: async!
beforeAll(async () => await remote.listen())
afterEach(() => remote.resetHandlers())
afterAll(async () => await remote.close())

The .listen() and .close() methods of the remote server become async since they now establish and terminate an internal server instance respectively.

Similar to the setupServer integration, it would be recommended to call setupRemoteServer once as a part of your global testing setup. Closing the WebSocket server after each test suite will have performance implications since each next test suite would wait while remote.listen() spawns that server again.

You can then operate with the remote server as you would with a regular setupServer, keeping in mind that it doesn't affect the current process (your test) but instead, any remote process that runs setupServer (your app).

it('handles user errors', () => {
  // Appending and removing request handlers is sync
  // because they are stored in the current (test) process.
  remote.use(
    http.get('/user', () => {
      return new Response(null, { status: 500 })
    })
  )

  // ...interact and assert your app.
})

By fully extending the SetupApi, the setupRemoteServer API provides the user with full network-managing capabilities. This includes defining initial and runtime request handlers, as well as observing the outgoing traffic of a remote process using the Life-cycle API (remote.events.on(event, listener)). I think this is a nice familiarity that also provides the user with more power when it comes to controlling the network.

Implementation

I've considered multiple ways of implementing this feature. Listing them below.

(Chosen) WebSocket server

The setupRemoteServer API can establish an internal WebSocket server that can route the outgoing traffic from any server-side MSW instance anywhere and deliver it to the remote server to potentially resolve.

Technically, the WebSocket server acts as a resolution point (i.e. your handlers) while the remote MSW process acts as a request supplier (similar to how the Service Worker acts in the browser).

Very roughly, this implies that the regular setupServer instances now have a fixed request handler that tries to check if any outgoing request is potentially handled by an existing remote WebSocket server:

// setupServer.js
await handleRequest(
  request,
  requestId,
  [
    // A very basic idea on how a "remote" request handler works.
    http.all('*', async ({ request }) => {
      wsServer.emit('request', serializeRequest(request))
      await wsServer.on('response', (serializedResponse) => {
        return deserializeResponse(serializedResponse)
      })
    }),
    ...this.currentHandlers,
  ]
)

Unlike request handler (i.e. function) serialization, it is perfectly fine to serialize Request and Response instances and transfer them over any message channel, like a WebSocket transport.

If no WebSocket server was found or establishing a connection with it fails within a sensible timeout period (~500ms), the setupServer instance of the app continues to operate as normal.

Alternatively, we can skip the WebSocket server lookup altogether and make it opt-in via some remote: true option on the app's side.

IPC

The test process and the app process can utilize IPC (interprocess communication) to implement a messaging protocol. Using that protocol, the app can signal back any outgoing requests and the test can try resolving them against the request handlers you defined immediately in the test.

This approach is similar to the WebSocket approach above with the exception that it relies on IPC instead of a standalone running server. With that, it also gains its biggest disadvantage: the app process must be a child process of the test process. This is not easy to guarantee. Depending on the framework's internal implementation, the user may not achieve this parent/child relationship, and the IPC implementation will not work.

Given such a demanding requirement, I've decided not to use this implementation.

Limitations

  • useRemoteServer() affects the network resolution for the entire app. This means that you cannot have multiple tests that override request handlers for the same app at the same time. I think this is more than reasonable since you know you're running 1 app instance that can only behave in a single way at a single point in time. Still, I expect users to be confused when they parallelize their E2E tests and suddenly see some network behaviors leaking across the test cases.

Concerns

  • Can we rely on a fixed local port to always be available?
  • Is it safe to introduce a WebSocket server that will be, effectively, routing HTTP messages over the local network (during tests only)?
    • Yes. If someone can intercept that WebSocket communication, they are already in your machine and can do things far worse than that.
  • Is it clear that setupRemoteServer only affects the server-side network behavior of any running application process with the server-side MSW integration? To affect the client-side network behavior from a test you have to 1) have setupWorker integration in the app; 2) set a global window.worker instance; 3) use window.worker.use() to add runtime request handlers. This stays as it is right now, no changes here.

The API is TBD and is subjected to change.

Roadmap

  • Ensure the sync server connection is awaited before the first request handler runs.
  • Introduce serialize/deserialize utilities for requests and responses (used both in the worker and in the WS sync layer now).
  • Fix listeners' memory leaks on hot updates (clean up listeners).
  • Make the WS events map type-safe
  • Rely on the internal request header when bypassing Socket IO connection requests in the rest.all() handler.
  • Handle socket timeout and errors when awaiting for the response in setupServer.
  • Support ReadableStream from the remote request handler (may consider transferring ReadableStream over the WS messages instead of ArrayBuffer, if that's allowed).
    • This may not be needed, in the end, but if we can pull off ReadableStream transfer over WebSockets that would be great.
  • Support all Life-cycle events.
  • Support setting a custom WebSocket server port number through environment variables.
  • Make the remotePort and port an implementation detail of setupRemoteServer and setupServer({ remote: true }). The developer mustn't care about those.
  • Do not spread the list of user-defined request handlers to prepend the fixed remote server handler (spreading of large lists may have performance implications).
    • Not an issue until proven otherwise; have no wish to optimize prematurely.
  • Solve the test/app catch 22 by attaching a self-replicating one-time handlers only for the first-time requests (those fetched when the testing framework pings your app).
  • Fix: failing use() test (may have something to do with the handlers management refactoring as a part of the server.boundary()).
  • Support differentiating between requests done in different Playwright workers (see this).
  • Add more tests, specifically for different response body types.
  • Consider adding setupWorker support (see feat: support cross-process interception via setupRemoteServer #1617 (comment)).
  • Consider dropping socket.io in favor of ws if we don't need anything socketio-specific
  • Silent WebSocket connection errors if the remote mode is enabled but the WebSocket server is not running/fails to connect. Print a warning and continue in regular mode.
  • Don't send life-cycle events for the WebSocket connection HTTP request to the WebSocket server (forwardLifeCycleEvents()).

Blockers

@kettanaito
Copy link
Member Author

@SebastianSedzik, yes, the intention is to iterate on the current design to allow multiple remote connections to the same setupServer instance. There are still problems with differentiating between those connections as there is no way to know what test has triggered a request to respond to it appropriately.

@arjenbloemsma,

Will this feature also allow to mock calls made by Next.js api routes?

This feature will allow you to control the network in one Node.js (e.g. the server-side of your Next.js) from another (e.g. your test).

So imagine I have a client component that calls a nextjs api route on the server, which in turn will make a call to a third party app. I would like to intercept the call to the 3rd party app with MSW and mock it.

For in-application usage, this already works. Check out this Next.js + MSW example for the full setup to enable this.

@kettanaito
Copy link
Member Author

I have some thoughts regarding the application spawn order and identifying individual tests/workers. Will share once I have a prototype confirming my ideas. Looks promising.

@spuxx1701
Copy link

This is really great. Thank you for your hard work! 🎉

@kettanaito
Copy link
Member Author

The tests are failing because I'm on pnpm 9 and #2211 isn't merged yet.

@kettanaito
Copy link
Member Author

kettanaito commented Dec 4, 2024

Blockers

@kettanaito
Copy link
Member Author

Update

Alright, there's a lot to unpack. In short, I've got the MVP of CPRI completely working. This includes the test/app paradox solved, the association between the app's runtime and a particular test/worker closure, and the ability to control the server-side network from the test.

Documenting those decisions for posterity.

Test/app paradox

Solved by two things:

  1. You have to build your app before the entire test run. Use the global setup hooks to achieve that.
  2. You have to start individual instances of your built app in each individual test case.

Not only does this solve the test/app paradox because it guarantees the fixed order of things (first test, then remote, then your app spawns), it allows the app to be spawned with a custom contextId, which is important for the next thing.

Test/app binding

Even when spawned within individual test cases, your app still communicates with a single instance of remote (to save bandwidth on spawning a ton of WebSocket servers, those are redundant). This makes remote a shared state across different test scenarios, which isn't nice (but it is nice to have it as a shared fallback!).

This is solved by using the remote.boundary() API. Similar to the existing server.boundary(), the remote boundary utilizes AsyncLocalStorage in Node.js to bind request handler overrides to the test's closure, preventing the shared state issue and allowing for concurrent test runs. In the remote context, the boundary has additional behavior because request resolution doesn't happen within the test's closure like it does for regular Node.js tests. Instead, it happens in the request listener of the WebSocket server, which is "visually" in await remote.listen().

Test/app binding occurs due to remote.contextId grabbing the unique ID of the particular boundary and storing it in a Map<ContextId, () => Context> internal map of the remote. During the request resolution (i.e. the request event handling), the API can lookup any async context by its ID and get the list of relevant handlers. Only those handlers will be used (the happy-path handlers from setupRemoteServer are still applied as with server.boundary()).

What's next?

A ton of todo's to cover! The public API isn't the most ergonomic thing, and I'm considering simplifying it quite a bit while exposing the customization options to the end developer. There are also some security considerations I want to address, mostly concerning safe defaults of the WebSocket server CORS.

This is also a good time to sponsor this effort. Thank you.

@kettanaito
Copy link
Member Author

kettanaito commented Dec 12, 2024

Bug: Empty buffer sent to WebSocket connection

[setupRemoteServer] ws client ERROR! Error: got binary data when not reconstructing a packet
    at Decoder.add (/msw/node_modules/.pnpm/socket.io-parser@4.2.4/node_modules/socket.io-parser/build/cjs/index.js:161:23)
    at Client.ondata (/msw/node_modules/.pnpm/socket.io@4.7.5/node_modules/socket.io/dist/client.js:182:26)
    at Socket.emit (node:events:518:28)
    at Socket.onPacket (/msw/node_modules/.pnpm/engine.io@6.5.4/node_modules/engine.io/build/socket.js:120:22)
    at WebSocket.emit (node:events:518:28)
    at WebSocket.onPacket (/msw/node_modules/.pnpm/engine.io@6.5.4/node_modules/engine.io/build/transport.js:94:14)
    at WebSocket.onData (/msw/node_modules/.pnpm/engine.io@6.5.4/node_modules/engine.io/build/transport.js:103:14)
    at WebSocket.<anonymous> (/msw/node_modules/.pnpm/engine.io@6.5.4/node_modules/engine.io/build/transports/websocket.js:20:19)
    at WebSocket.emit (node:events:518:28)
    at Receiver.emit (node:events:518:28)
    at Receiver.dataMessage (/msw/node_modules/.pnpm/ws@8.11.0/node_modules/ws/lib/receiver.js:514:14)
    at Receiver.getData (/msw/node_modules/.pnpm/ws@8.11.0/node_modules/ws/lib/receiver.js:446:17)
    at Receiver.startLoop (/msw/node_modules/.pnpm/ws@8.11.0/node_modules/ws/lib/receiver.js:148:22)
    at Receiver._write (/msw/node_modules/.pnpm/ws@8.11.0/node_modules/ws/lib/receiver.js:83:10)
    at writeOrBuffer (node:internal/streams/writable:564:12)
    at _write (node:internal/streams/writable:493:10)
    at Receiver.Writable.write (node:internal/streams/writable:502:10)
    at Socket.socketOnData (/msw/node_modules/.pnpm/ws@8.11.0/node_modules/ws/lib/websocket.js:1272:35)
    at Socket.emit (node:events:518:28)
    at addChunk (node:internal/streams/readable:559:12)
    at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)
    at Socket.Readable.push (node:internal/streams/readable:390:5)
    at TCP.onStreamRead (node:internal/stream_base_commons:190:23)
    at TCP.callbackTrampoline (node:internal/async_hooks:130:17)

No idea why this happens. Something down the line sends an empty buffer to the server, SocketIO tries to emit the message with that buffer, and fails on the top-most frame.

Root cause

this.forwardLifeCycleEvents()

Forwarding the response:bypass event for the WebSocket handshake to the remote causes this error:

{
  type: 'response:bypass',
  payload: {
    response: {
      __serializedType: 'response',
      status: 101,
      statusText: 'Switching Protocols',
      headers: [Array],
      body: [ArrayBuffer]
    },
    request: {
      __serializedType: 'request',
      method: 'GET',
      url: 'http://localhost:56957/socket.io/?EIO=4&transport=websocket',
      headers: [Array],
      body: undefined
    },
    requestId: '16ddfd8d917a6'
  }
}

Note that body of the response if an empty ArrayBuffer. Sending an empty buffer errors the SocketIO parsing phase.

@kettanaito
Copy link
Member Author

kettanaito commented Dec 12, 2024

Bug: Deep objects cannot be emitted with SocketIO

Not sure if it's MSW interfering in some way, but emitting the request event with the deep object as an argument never arrives to the server. Shallow objects work fine.

Would be nice to add an integration test to Interceptors to confirm or debunk this.

✅ Solved

Moved away from WebSockets to HTTP for the internal server.

@kettanaito
Copy link
Member Author

Test failures

 FAIL  test/node/msw-api/setup-remote-server/response.body.test.ts [ test/node/msw-api/setup-remote-server/response.body.test.ts ]
 Test Files  2 failed | 84 passed (86)
Error: listen EADDRINUSE: address already in use ::1:56957

This is caused by migrating to HTTP for the internal server. Since Vitest likely runs some of these tests in parallel, it attempts to call remote.listen(), which attempts to create multiple internal HTTP servers on the same port, which, naturally, fails.

This is a good precursor into figuring out whether the internal server port is fixed or random. I likely lean toward making it random and passing it along with remoteContext through remote.boundary() (but that will make the boundary required; or we can use a fallback port).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.