fix(server-runtime: fix request signal not aborting after request is garbage collected

[jvaill]

Closes: #9976 (comment) Edit: There appears to be a different bug with signals not working at all in production.

Update (Sep 26)

In development, it appears that globals aren't being automatically installed and Request with node ends up coming from undici. undici currently has a bug where parent signals aren't properly followed: nodejs/node#55428

In production, globals are installed and Request comes from web-fetch. This version of Request does not have the above issue.

It seems like globals should be installed in development as well? Manually invoking installGlobals() changes the Request object and solves this issue.

Original:

Testing Strategy:

Add remix-utils and the following routes. Start the dev server and open /time in a bunch of different tabs. Refresh the tabs randomly, close them, create new ones, etc, and eventually you'll trigger the GC in the right way where SSE connection closed ${number of open conns} will be logged to the console with the number of open connections being greater than the number of open tabs. A good way to see if you've triggered this is just to close all of them and see if the logged connection count is greater than 0.

routes/sse.time.ts

import { LoaderFunctionArgs } from "@remix-run/node"
import { eventStream } from "remix-utils/sse/server"

let conns = 0;

export async function loader({ request }: LoaderFunctionArgs) {
  conns++;
  return eventStream(request.signal, function setup(send) {
    const timer = setInterval(() => {
      send({ event: "time", data: new Date().toLocaleTimeString() })
    }, 1000)

    return function clear() {
      conns--;
      console.log("SSE connection closed", conns);
      clearInterval(timer)
    }
  })

routes/time.tsx

import { useEventSource } from "remix-utils/sse/react"

export default Route(){
  let time = useEventSource("/sse/time", { event: "time" })
  if (!time) {
    time = new Date().toLocaleTimeString()
  }

return <p>{time}</p>

[changeset-bot]

🦋 Changeset detected

Latest commit: aaab009

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages

Name	Type
@remix-run/server-runtime	Patch
@remix-run/cloudflare	Patch
@remix-run/deno	Patch
@remix-run/dev	Patch
@remix-run/node	Patch
@remix-run/react	Patch
@remix-run/testing	Patch
@remix-run/cloudflare-pages	Patch
@remix-run/cloudflare-workers	Patch
@remix-run/architect	Patch
@remix-run/express	Patch
@remix-run/serve	Patch
create-remix	Patch
remix	Patch
@remix-run/css-bundle	Patch
@remix-run/eslint-config	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[remix-cla-bot]

Hi @jvaill,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[jvaill]

Fyi - the underlying problem appears to be a bug in undici:

nodejs/node#55428

[brophdawg11]

It seems like globals should be installed in development as well? Manually invoking installGlobals() changes the Request object and solves this issue.

FWIW we are deprecating the @remix-run/web-fetch package in favor of using the built-in fetch on Node 20 and above - something you can opt-into via installGlobals({ nativeFetch: true }) and is usually done when you opt into Single Fetch (see https://remix.run/docs/en/main/guides/single-fetch#breaking-changes).

For now, it looks like there is some traction on the node/undici end so do we think they'll be getting a fix out anytime soon? It would be nice not to have to put this type of workaround/hack into the Remix codebase to fix a bug in undici/node...

[jvaill]

@brophdawg11 Another update on this - (IIRC, been a few weeks) at first I thought this was going to fix signals in production, but it seems like that's an entirely different bug altogether. When signals are working (i.e., in development), this fixes signals sporadically not being aborted due to GC.

I gave up on getting signals to work and ended up switching from SSE to websockets 😅

[Eiley2]

@brophdawg11 Another update on this - (IIRC, been a few weeks) at first I thought this was going to fix signals in production, but it seems like that's an entirely different bug altogether. When signals are working (i.e., in development), this fixes signals sporadically not being aborted due to GC.

I gave up on getting signals to work and ended up switching from SSE to websockets 😅

Did you implement your own server with Express and Remix? Or what did you use? Considering to do the same tbh

[mkromann]

Is it correct to say that SSEs are not working right now in Remix? I have been trying the last few days to find a workaround to this but been unable. We are starting up a new project and this is a curveball. It would be sad to go another route than Remix!

These issues are also reported here:
sergiodxa/remix-utils#317
dan-cooke/remix-sse#27

[mkromann]

@brophdawg11 Have you considered the path forward regarding this issue? Is this a waiting thing, or something you can/will patch?

[jvaill]

@brophdawg11 Another update on this - (IIRC, been a few weeks) at first I thought this was going to fix signals in production, but it seems like that's an entirely different bug altogether. When signals are working (i.e., in development), this fixes signals sporadically not being aborted due to GC.
I gave up on getting signals to work and ended up switching from SSE to websockets 😅

Did you implement your own server with Express and Remix? Or what did you use? Considering to do the same tbh

I ended up adding a separate service to handle websockets. Basically, it uses createServer (node:http) and upgrade the connection to a websocket via WebSocketServer (ws). I use redis pub/sub to trigger messages from the remix app that get picked up by the service and proxied to the client. Might be a bit overly complex depending on your needs.

[github-actions]

Thank you for opening this pull request, and our apologies we haven't gotten around to it yet!

With the release of React Router v7 we are sun-setting continued development/maintenance on Remix v2. If you have not already upgraded to React Router v7, we recommend you do so. We've tried to make the upgrade process as smooth as possible with our Future Flags. We are now in the process of cleaning up outdated issues and pull requests to improve the overall hygiene of our repositories.

We plan to continue to address 2 types of issues in Remix v2:

• Bugs that pose security concerns
• Bugs that prevent upgrading to React Router v7

If you believe this pull request meets one of those criteria, please respond or create a new pull request so it pops up on our radar (since github notifications may get lost in the noise 😕).

For all other issues/changes, ongoing maintenance will be happening in React Router v7, so:

• If this is a bug fix, please reopen against React Router v7
• If this is a new feature, please open a new Proposal Discussion in React Router, and if it gets enough community support it can be considered for implementation

If you have any questions you can always reach out on Discord. Thanks again for providing feedback and helping us make our framework even better!