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

Jest reports BroadcastChannel leak #52

Closed
erictheswift opened this issue Jan 14, 2023 · 4 comments · Fixed by #53
Closed

Jest reports BroadcastChannel leak #52

erictheswift opened this issue Jan 14, 2023 · 4 comments · Fixed by #53

Comments

@erictheswift
Copy link
Contributor

erictheswift commented Jan 14, 2023

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch lib0@0.2.52 for the project I'm working on.

After I updated to node@18 and jest@28 I found out jest reports some of my tests leak.

Jest has detected the following 1 open handle potentially keeping Jest from exiting:

  ●  MESSAGEPORT
      at node_modules/lib0/broadcastchannel.js:66:16
      at Object.setIfUndefined (node_modules/lib0/map.js:49:24)
      at map.setIfUndefined (node_modules/lib0/broadcastchannel.js:64:3)
      at Object.subscribe (node_modules/lib0/broadcastchannel.js:83:39)
      at WebsocketProvider.bc.subscribe [as connectBc] (node_modules/y-websocket/src/y-websocket.js:345:7)
      at WebsocketProvider.connect (node_modules/y-websocket/src/y-websocket.js:394:12)
      at new WebsocketProvider (node_modules/y-websocket/src/y-websocket.js:305:12)

I found out that broadcastchannel module keeps BroadcastChannel alive for every room even with zero subscribers.

Here is the diff that solved my problem:

diff --git a/node_modules/lib0/dist/broadcastchannel-7655b66f.cjs b/node_modules/lib0/dist/broadcastchannel-7655b66f.cjs
index 12a6f51..fc82374 100644
--- a/node_modules/lib0/dist/broadcastchannel-7655b66f.cjs
+++ b/node_modules/lib0/dist/broadcastchannel-7655b66f.cjs
@@ -74,7 +74,17 @@ const subscribe = (room, f) => getChannel(room).subs.add(f);
  * @param {string} room
  * @param {function(any, any):any} f
  */
-const unsubscribe = (room, f) => getChannel(room).subs.delete(f);
+const unsubscribe = (room, f) => {
+    const channel = getChannel(room);
+    const unsubscribed = channel.subs.delete(f);
+    if (unsubscribed) {
+        if (channel.subs.size === 0) {
+            channel.bc.close();
+            channels.delete(room);
+        }
+    }
+    return unsubscribed;
+}
 
 /**
  * Publish data to all subscribers (including subscribers on this tab)

This issue body was partially generated by patch-package.

@dmonad
Copy link
Owner

dmonad commented Jan 16, 2023

Thanks for the PR @erictheswift. I'll release a patch in a bit

@dmonad dmonad closed this as completed Jan 16, 2023
@erictheswift
Copy link
Contributor Author

https://github.com/dmonad/lib0/actions/runs/3931623661/jobs/6723124025#step:6:105 2.0.59 patch build failed because of temporal test machine slowdown?

@dmonad
Copy link
Owner

dmonad commented Jan 16, 2023

I think node made a certain function faster. You don't have to worry about it.

@dmonad
Copy link
Owner

dmonad commented Jan 16, 2023

The function is still slower 90% of the time. Eventually, I'll move to the native encoding approach.

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 a pull request may close this issue.

2 participants