Federation performance and bug fixes

[hyfen]

Hello RocketChat! @ndroo and I have been working on bug fixes and performance improvements for federated rooms. This PR gathers a bunch of our changes (we can split up into individual PRs if preferred).

A summary of our changes:

• memoize DNS responses so the checks aren't being done every time a new event arrives
• brings in missing behaviour for threaded messages for federated messages
• improves performance of dispatching system by only sending the same events to a domain once
• fixes a missing import in app/federation/server/hooks/afterUnsetReaction.js
• makes the max number of federated domains configurable through an environment variable
• reduces number of times we have to run federationServers.refreshServers();

In our repo, we also cherry-picked the unmerged #16430 which is working well.

[CLAassistant]

All committers have signed the CLA.

[lgtm-com]

This pull request introduces 2 alerts when merging 3771d95 into 2808ce7 - view on LGTM.com

new alerts:

• 2 for Invocation of non-function

[sampaiodiego]

hey, thanks for the contribution.. looks like you guys have been using federation a lot, which is great :D

I have asked to change all _.uniq for [...new Set()], which is faster and doesn't rely on any external dependency.

I saw that are other lint related issues, can you please look at them? thanks again for the awesome work

[lgtm-com]

This pull request introduces 2 alerts when merging 0b405ec into 2808ce7 - view on LGTM.com

new alerts:

• 2 for Invocation of non-function

[hyfen]

@sampaiodiego Switched to using Set and fixed the linting issues.

Also the linting pointed me to a bug that I had introduced in an earlier refactoring. Fixed that here: 7e225da#diff-2ddd532f087a155b46f014ddedcf3102R71

[sampaiodiego]

@sampaiodiego Switched to using Set and fixed the linting issues.

Also the linting pointed me to a bug that I had introduced in an earlier refactoring. Fixed that here: 7e225da#diff-2ddd532f087a155b46f014ddedcf3102R71

oops, that was my fault, bad suggestion :/

but thanks again.. 👍

[alansikora]

Great work! We are preparing some changes to the next major version of Rocket.Chat, some of this files might change, but I will definitely use some of this PR. Thanks for the contribution!

[ndroo]

Hey @sampaiodiego I was the author of this change that @hyfen submitted. This await is here intentionally. The challenge is that due to the way the federation system works, if a server is backlogged in past events, it might get sent the same event multiple times. When this occurs, if you try and insert the message a second time, it'll fatally fail with a duplicate key error. Being able to catch this is critical in ensuring the other messages process.

My suggestion is just to add it back, or ensure this scenario is handled in some other capacity.

[sampaiodiego]

thanks @ndroo for the explanation.. I see your point, but that await was not helping to prevent that to happen. await is effective only when using Promises, that is not the case for Messages.insert.. actually, any model action is already synchronous due to Meteor's Fibers (except for raw models, that doesn't use Meteor's collections thus doesn't rely on Fibers), that's why I removed the await, the behaviour should stay the same.

The challenge is that due to the way the federation system works, if a server is backlogged in past events, it might get sent the same event multiple times. When this occurs, if you try and insert the message a second time, it'll fatally fail with a duplicate key error.

@alansikora any way to prevent this from happening?

[ndroo]

Hmm ok yeah, i'm honestly very new to all this but the situation seemed to have been resolved when i added it. Perhaps I didn't have a reliable enough test though?

I guess using upsert v's insert could also mitigate the issue.

[sampaiodiego]

maybe you can share how you tested with us so we can try reproduce it too and think on other ways to fix it? I don't want to merge a buggy code by no means, specially one that I have changed without testing properly =)

using upsert could do the tricky, but just like the try/catch it can possibly hide an unwanted behaviour.. I had a quick discussion with @alansikora and he mentioned this try/catch might not be there.. as if some error happens we maybe shouldn't continue.

[ndroo]

Yeah, so i see the point, it should probably check for just a mongo duplicate key. That issue was breaking federation on a regular basis for me in testing. It’s pretty simple to expose the crash, connect two servers, and mashing the keyboard on both at the same time make a bunch of messages. You’ll get a crash due to duplicate keys (same event being processed multiple times) in a matter of seconds. Edit: I said MySQL, i meant mongo. Whoops!

[lgtm-com]

This pull request introduces 2 alerts when merging a31c887 into b83cbf9 - view on LGTM.com

new alerts:

• 2 for Invocation of non-function