Risk for memory leaks with subscriptions on page reload
NordlingDev opened this issue · 4 comments
Sometimes during development it's necessary to fully reload the page. I created this little subscription procedure to see whether there is a risk for memory leaks. And yes, there is. Check the code below:
let count = 0;
export const testSubscription = publicProcedure.subscription(async () => {
return observable<string>(() => {
count ++;
console.log(count);
return () => {
count --;
};
});
});
Whenever this procedure is being subscribed to, count
will increment or decrement to keep track of amount of active subscriptions. However, if you do a page reload, there is no cleanup process being made and count
keeps increasing.
I fixed this with a simple workaround inside the app itself (React app) by having a beforeunload
listener on the window to unmount the app before the page actually reloads. See below:
import { StrictMode, useEffect } from "react";
import * as ReactDOM from "react-dom/client";
import { Root } from "./app";
const root = ReactDOM.createRoot(document.getElementById("root") as HTMLElement);
window.addEventListener("beforeunload", () => {
root.unmount();
});
root.render(
<StrictMode>
<Root />
</StrictMode>,
);
Now, I'm not sure if this is the prettiest way to do it and neither am I sure it covers all potential cases. So is this is something worth investigating?
Hey, so I looked into this and I don't think there's an actual leak happening here.
When we create new subscriptions for a given window, we'll actually overwrite existing subscriptions for that window when the new ones are created. In some ways the behavior is the same as tRPC over browser+server, but the key difference is that a tRPC server can catch broken websocket connections and perform cleanup. I think electron-trpc can do this as well, but we don't currently.
@jsonnull Yeah there is no leak actually happening. I was just saying there is a risk for it if devs aren't aware that no cleanup is being made to subscriptions when "connections" are lost (due to page reload in this case).