jsonnull / electron-trpc

Build type-safe Electron inter-process communication using tRPC
https://electron-trpc.dev/
MIT License
267 stars 26 forks source link

Subscriptions do not cleanup #124

Closed spencekim closed 1 year ago

spencekim commented 1 year ago

Using your example repo, I noticed that subscriptions do not clean up. Eventually I get the following error:

[1] (node:22811) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 output listeners added to [EventEmitter]. Use emitter.setMaxListeners() to increase limit
[1] (Use `Electron --trace-warnings ...` to show where the warning was created)
[1] (node:22811) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 destroyed listeners added to [EventEmitter]. Use emitter.setMaxListeners() to increase limit
[1] subscribed
spencekim commented 1 year ago

In /electon/api.ts, if you console.log in the cleanup function within the observable, you will see that the component unmounts without cleaning up.

jsonnull commented 1 year ago

Hi, thanks for reporting the issue! I'll take a look at this and see what's happening.

spencekim commented 1 year ago

were you able to confirm this behavior?

biw commented 1 year ago

It looks like the return value in the subscription observable never gets called. Can reproduce it in the example/basic-react folder:

  return () => {
        ee.off('greeting', onGreet);
        console.log('removed event onGreen ');
      };

The function is only fired once the electron app closes rather than when the component that uses the useSubscription function is unmounted. Currently looking into the code to find out more if you have any ideas, @jsonnull, I will make a PR with the changes once I find it.

biw commented 1 year ago

So here's what I found. When a subscription is done it fires off an operation to handleIPCOperation:

{ id: 1, method: 'subscription.stop' }

with a sequential id, however when this operation is passed into callProcedure, it throws an error since the path is undefined:

I: No "undefined"-procedure on path "undefined"

This prevents the complete call from firing in result.subscribe, so the client never gets the "stopped" response. It also never calls subscription.unsubscribe() and the only place that calls that is later in handleIPCOperation:

event.sender.on('destroyed', () => subscription.unsubscribe());

which is valid and cleans up the subscription when the event.sender is destroyed, but this will only happen when the electron window is destroyed, either by closing it or stopping the electron app. This is what's causing the memory leak as the subscriptions are never .unsubscribe()'d from until the app is closed.

I couldn't seem to find a way to detect that the component on the frontend was was unloaded so that's where I got stuck.

jsonnull commented 1 year ago

So I looked into this and I've found a pretty big oversight in the subscription architecture on the server-side. There's multiple issues here that were converging.

As Ben's pointed out (thanks for making this part easy!) the subscription.stop message is sent to the server, but the server isn't set up to handle this correctly.

It turns out that there's more than that. As Ben also points out, each new subscription was setting a handler on the window to clean up the subscription when the window is destroyed. This is the leak. What's worse is, since the subscription.stop method wasn't being handled, not only was the subscription not getting cleaned up, but this listener on the window's EventEmitter wasn't getting cleaned up either.

I've got a fix working where the subscription cleanup runs, and there's only one listener per window for the window destruction no matter how many subscriptions run.

So I just need to clean up that fix, check on tests, and I'll push the fix out.

spencekim commented 1 year ago

Very glad you found the issue. I'll give it a test once it's merged in, hopefully soon!

jsonnull commented 1 year ago

Alright, electron-trpc@0.5.0 is out with a fix. If you get a chance, let me know if things are working better for you!

spencekim commented 1 year ago

Confirming that it works! Thanks!