Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flood protection #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

davidscholberg
Copy link
Owner

The flood protection code seems to have been overhauled in matrix-org#40. The approach to flood protection taken in that PR seems to be to create a timed promise based on the value of the lastSendTime variable, and then to create a new promise which waits on the old send promise and the new timed promise before sending the message and then updating the lastSendTime variable.

The problem with this code is that if several calls to the send function are made inside the same event (which is what happens when, for example, hubot responds to the help command and there are several commands in the output, one per line), all of the timed promises made in the send function will be created using the exact same value of the lastSendTime variable. The reason for this is that the await calls split the execution flow and put the code after the await call onto the microtask queue, and the microtask queue will not be executed until the current event has finished. So as a result, the lastSendTime variable will not be updated at all until all of the timed promises have already been created, and if the time elapsed since lastSendTime was updated is greater than the flood protection delay, the timed promises will all be zero seconds long and there will effectively be no flood protection.

The approach to flood protection taken in this commit is to use chained promises which alternate between sending and waiting. The sendingPromise field is kept and represents the latest promise in the promise chain. When a new send call is made, it is immediately chained to the previous sending promise and appended with a timed promise that lasts the duration of the flood protection delay. The sendingPromise field is then updated to this new promise so that subsequent calls to send can be chained to it. Since we are always appending a timed promise the exact duration of the flood protection delay to the send call, and chaining all subsequent send calls, we don't have to do any math to figure out how much time is left before a message can be sent, and as such, the lastSendTime and MIN_DELAY_MS variables have been removed.

The flood protection code seems to have been overhauled in
matrix-org#40. The approach to flood
protection taken in that PR seems to be to create a timed promise based
on the value of the lastSendTime variable, and then to create a new
promise which waits on the old send promise and the new timed promise
before sending the message and then updating the lastSendTime variable.

The problem with this code is that if several calls to the send function
are made inside the same event (which is what happens when, for example,
hubot responds to the help command and there are several commands in the
output, one per line), all of the timed promises made in the send
function will be created using the exact same value of the lastSendTime
variable. The reason for this is that the await calls split the
execution flow and put the code after the await call onto the microtask
queue, and the microtask queue will not be executed until the current
event has finished. So as a result, the lastSendTime variable will not
be updated at all until all of the timed promises have already been
created, and if the time elapsed since lastSendTime was updated is
greater than the flood protection delay, the timed promises will all be
zero seconds long and there will effectively be no flood protection.

The approach to flood protection taken in this commit is to use chained
promises which alternate between sending and waiting. The sendingPromise
field is kept and represents the latest promise in the promise chain.
When a new send call is made, it is immediately chained to the previous
sending promise and appended with a timed promise that lasts the
duration of the flood protection delay. The sendingPromise field is then
updated to this new promise so that subsequent calls to send can be
chained to it. Since we are always appending a timed promise the exact
duration of the flood protection delay to the send call, and chaining
all subsequent send calls, we don't have to do any math to figure out
how much time is left before a message can be sent, and as such, the
lastSendTime and MIN_DELAY_MS variables have been removed.
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.

1 participant