Fix leaking listeners

[bimusiek]

What, How & Why?

If Realm is inTransaction and listener is created in the setImmediate, it won't be removed from listeners if tearDown is called quickly (which happens often if one listener triggers re-render of other listeners)

This closes #1099 (comment)

☑️ ToDos

• 📝 Changelog entry
• 📝 Compatibility label is updated or copied from previous entry
• 📝 Update COMPATIBILITY.md
• 🚦 Tests
• 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
• 📱 Check the React Native/other sample apps work if necessary
• 💥 Breaking label has been applied or is not necessary

[cla-bot]

Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @bimusiek. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a @cla-bot check comment. The GitHub usernames you file there will need to match that of your Pull Requests. If you have any questions or cannot file the CLA electronically, make a comment here and we will be happy to help you out.

[bimusiek]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[nirinchev]

Hey @bimusiek thank you so much for the contribution. We'll try and get this reviewed shortly, but in the meantime, I was wondering if you have considered an approach similar to C#'s cancellation tokens - namely setting a flag that can be checked in the setImmediate callback that would skip the listener registration altogether. My understanding is that your use case involves rapid registrations/unregistrations and I can imagine there might be a moderate performance gain if we avoid the work altogether. JS is not my strongest language, but I'm thinking something that looks roughly like this:

let registrationState: "registered" | "torn-down" | "pending" = "pending";

// ...
if (realm.isInTransaction) {
  setImmediate(() => {
    if (registrationState == "pending") {
      collection.addListener(listenerCallback, keyPaths);
      registrationState = "registered"
    }
  });
}

// ...

const tearDown = () => {
  // ...
  if (registrationState == "registered") {
    collection.removeListener(listenerCallback);
  }
  registrationState = "torn-down";
};

[bimusiek]

@nirinchev This is good idea, I will implement it soon.

We have release the app to production with this change and it helped a lot. No more memory leaks due to listener callbacks leaks. However, the impact of this change was lesser than we thought, as we had another hook that was re-rendering a lot. Which triggered this scenario of leaking listener callback.

So at the end, this issue should not have huge impact.

The re-rendering issue we had, was related to Realm.BSON.ObjectID which is changing reference even if the hexString value did not change. I would like to open discussion about making ObjectID cached so React won't rerender components if you use ObjectID inside the deps. Unfortunately this looks like deeper Realm integration and I have no idea how to approach this.

For now, we have patched react typings to disallow ObjectID in the deps so Typescript notifies us if someone uses ObjectID directly. And we rewrote most of the code to operate on hexString. But this seems like a huge oversight if Realm wants to integrate nicely with React.

[elle-j]

@bimusiek, we've added your ObjectId cache suggestion in this ticket. Feel free to add anything else you think is relevant.

[kneth]

LGTM

I have tested locally (since GHA doesn't spin up the server), and I see the same result as testing on main.

[elle-j]

Thank you for your contribution @bimusiek! We will link to your profile in the CHANGELOG entry for this fix 👍