jeromeludmann / deno-irc

IRC client protocol module for Deno
https://deno.land/x/irc
MIT License
12 stars 4 forks source link

Implement optional flood protection #11

Closed aronson closed 9 months ago

aronson commented 9 months ago

Hello, I bring an enhancement we're implementing in discord-irc: flood protection. I figure it might make more sense to have upstream for other consumers of the library.

It's possible for clients to, say, naively chunk up a 4000 character input message in a for loop and get kicked from the IRC server when using the library. This provides a solution to that problem that is simple to apply in configuration.

This option utilizes the Queue library and is disabled by default.

All tests pass with a small change to allow the queue to process messages in the mock environment.

Documentation updated.

Tested and working as expected with discord-irc.

jeromeludmann commented 9 months ago

Hello, thank you for contributing.

Some first impressions/questions:

1/ I see this feature is restricted to PRIVMSG, would it make sense to push all outgoing IRC messages in the queue (rather than only PRIVMSG)?

2/ Need some tests/thoughts but I wonder if we could implement it "as a plugin" with something like:

// plugins/antiflood.ts

// `hookCall` is roughly like a patch function
client.hooks.hookCall("send", (send, command, ...params) => {
  if (command === "PRIVMSG") {
    return queue.push(async () => {
      const raw = await send(command, ...params);
      await delay(floodDelay);
      return raw;
    });
  } else {
    return send(command, ...params);
  }
});

the only reason would be to keep the core part easier (and secondly free from third party library). WDYT?

aronson commented 9 months ago

For the first part, I initially tried all messages and found it made the client quite slow to connect and register in a situation where those messages don't need to be restricted. I think there's a limited subset of IRC messages that should be rate limited but I'm not sure what all members in that set are. We noticed this when trying to send long split messages from Discord to IRC and were getting kicked from Libera in our tests.

For the second part, pushed the change. Much cleaner, I like the pattern you have established. This reveals we need some tests for the plugin, so I'm working on that next.

aronson commented 9 months ago

Tests pushed and passing, and application working as expected in my discord-irc integration tests.

jeromeludmann commented 9 months ago

I think it's ready to be merged now. Let me know when you are OK to merge.

aronson commented 9 months ago

I ran through my integration tests with the updated changes on my end and everything passes. OK to merge.