Fixes race condition in QuoteBoardForwarder

[Zabuzard]

Overview

QuoteBoardForwarder has a race condition that causes a message to be forwarded multiple times to #quotes if multiple users reacted to a message within short time. (This was already attempted with #1523 previously)

This PR fixes the problem by slightly redesigning the logic-flow in a way that the race condition cannot occur anymore by design.

Details

Previously, the onMessageReactionAdd handler decided whether a message will be forwarded or not. But this method is called concurrently by JDA on each reaction. So, by design, this is rather unsafe.

The change introduced in this PR is conceptionally simple. onMessageReactionAdd now only adds the message to a Set<ReactedMessage>, aka "messages to process" (like a Queue, but no duplicates).

From there, it is then picked up by a Routine once a minute and the decision and handling is now done there instead. The routine is set to ScheduleMode.FIXED_DELAY instead of FIXED_RATE, so by design it can only run after the previous routine run is done, not concurrently anymore. This fixes the underlying problem by design.

The rest of the changes are just cosmetic and code-style improvements without any logic changes.

Remarks

Checks

Originally I wanted to do more of the early-outs already in onMessageReactionAdd, so the messages ending up in the task-queue (Set<ReactedMessage>) are only messages that actually need to be forwarded to #quotes.

Like, ideally onMessageReactionAdd would already check:

• does the message still exist?
• was it already forwarded previously?
• does it even have a high enough emoji-count?

But sadly, due to JDA shenanigans, that isnt really possible. Well, it is possible, but it wouldnt yield any performance boost. The problem is that the MessageReactionAddEvent event does not provide a handle to the message yet. Accessing the message (to check if it exists, if it was already forwarded, or to compute its emoji scores) requires an event.retrieveMessage() there already, i.e. a Rest-Api call.

So doing these checks in the reaction-handler already would not save us anything and would actually lead to us having to do two API calls per message - not good.

Because of that those checks are all moved to the Routine and the reaction-handler itself only does the checks it can do right away without any API calls.

Lock

I would love to have avoided that private final Object lock around the set. Unfortunately, there is no better way to do an atomic "fetch and remove".

Like, we either need to do getAll-and-clear (drain) or getOne-and-remove (poll). Set doesnt offer this natively. Queue does, but that allows duplicates and we really dont want duplicates in this (since reactions usually come in waves).

There are of course also other workarounds, but I ultimately decided to add a small lock instead.