Use promise based flood delay improvements over a timer #40
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidscholberg
added a commit
to davidscholberg/node-irc
that referenced
this pull request
Nov 18, 2020
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.
This was referenced Nov 18, 2020
davidscholberg
added a commit
to davidscholberg/node-irc
that referenced
this pull request
Nov 18, 2020
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This code replaces our flood delay stuff. Previously we would run a timer func every second for all connections to see if a message is waiting to be sent. The new code now uses promises to achieve this, and in most cases should not activate. This hopefully saves us a lot of overhead.