Skip to content

Memory leak#51

Open
MurkyMeow wants to merge 1 commit intoshtaif:masterfrom
MurkyMeow:fix-memory-leak
Open

Memory leak#51
MurkyMeow wants to merge 1 commit intoshtaif:masterfrom
MurkyMeow:fix-memory-leak

Conversation

@MurkyMeow
Copy link

Hey! Absolutely solid work on this library! I've been using it extensively in my finance-related project and it's been a pleasure.

I've noticed a memory leak in my app and traced it down to this line in this library:

      await Promise.race([
            ownNextEventDeferred.promise, // <- this promise is reused, .then() chained onto it infinitely
            sharedNextEventDeferred.promise, // <- this promise is resolved and re-created, all ok
          ]);

I have it fixed here!

@shtaif
Copy link
Owner

shtaif commented Feb 5, 2026

Hey @MurkyMeow!
Really glad (and surprised) to learn that some one else but me actually found this valuable.
Kindly allow me just an extra day or two to sit down to fully comprehend and address this.
Thanks a lot for the help!! ⭐

@shtaif
Copy link
Owner

shtaif commented Feb 9, 2026

@MurkyMeow I appreciate the help your effort in pinpointing this! I always had such suspicion about such edge cases with promises but have never yet actually come to meet face to face like this.

I think I'd lean towards trying to preserve the "promise style". I've had some thinking and I'm pretty certain the leaking around Promise.race itself could be mitigated like this:

  1. in createMulticastChannel.ts:L44:
-      const ownNextEventDeferred = new Deferred<void>();
+      let ownNextEventDeferred = new Deferred<void>();
  1. then around createMulticastChannel.ts:L77:
        await Promise.race([
          ownNextEventDeferred.promise,
          sharedNextEventDeferred.promise,
        ]);

+       ownNextEventDeferred = new Deferred();

I think I managed to observe this is avoiding the accumulation. Would you be kind to check this approach through your scenario as well please?

Thanks! 🌠

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.

2 participants