minster586/nadekobot
1
0
Fork 0
mirror of https://gitlab.com/Kwoth/nadekobot.git synced 2025-09-10 17:28:27 -04:00
Code Issues Packages Projects Releases Wiki Activity

Merge branch 'rerorace' into 'v3'

Browse Source 
Fix possible race condition for reaction roles

### Description  
This MR aims to fix a possible race condition on the addition of exclusive reaction roles when the reactions are spammed by the user.

### Changes Proposed  
- Add a `ConcurrentHashSet<(ulong, ulong)>` to keep track of exclusive reaction roles that are being processed.
- Added logic that takes the collection above in consideration.
    - If entry is present, quit silently.
    - Else, perform the reaction role stuff then remove the entry.

### Details  
Exclusive reaction roles are meant to be exactly that - exclusive.

Normally, when a user selects an exclusive role they receive that role. If they select another role, their previous role is removed and the new one is added. There is a bug where if the user spams the reactions for a short period of time, Nadeko will eventually assign them multiple roles that are meant to be exclusive with each other. This happens because the events that handle the addition and removal work in a weird way - first they offload the removal of the roles to a `Task.Run()`, which also happens to have a `Task.Delay()` in it (possibly to avoid Discord ratelimits).

Concurrently, it proceeds to add the role that the user picked. The problem with this approach is that the Task that handles the role removal takes long enough for another reaction event to trigger and start the same work for a different reaction role. Then mayhem ensues, with different events concomitantly adding and removing the roles that previous events have removed or added. In the end, the user ends up with multiple exclusive roles they are not supposed to.

This MR fixes this by having a local field that keeps track of the reaction roles that are being currently processed (a `ConcurrentHashSet<T>` where T is a tuple `(ulong, ulong)` - (message ID, user ID)). When a reaction event runs, it adds itself to the concurrent hashset. If another event triggers and tries to add itself while the previous event still hasn't finished, it silently quits so it doesn't interfere with the current event. I was not entirely satisfied with the way this works, so I tried another system that cancels the old events (with a CancellationToken) instead of just making the new events quit, but that resulted in multiple exclusive roles being temporarily assigned to the user (just for a few seconds, but still).

If another approach is preferred, then please do let me know.

### Notes  
- Methods in that entire file need to be broken down into smaller methods.
- The loop that removes old reactions is **very slow**. Using `Task.WhenAll()` instead of awaiting the removals could help improve performance (but could also trigger ratelimits).

See merge request Kwoth/nadekobot!183
This commit is contained in:
Kwoth
2021-11-09 10:41:49 +00:00
parent 02c7ded457 1d57191700
commit c8e01bd158
1 changed files with 121 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files

186
src/NadekoBot/Modules/Administration/Services/RoleCommandsService.cs
View File

@@ -12,6 +12,8 @@ using LinqToDB;
using LinqToDB.EntityFrameworkCore;
using NadekoBot.Db;
using Serilog;
using System.Threading;
using System;
namespace NadekoBot.Modules.Administration.Services
{
@@ -21,6 +23,11 @@ namespace NadekoBot.Modules.Administration.Services
private readonly DiscordSocketClient _client;
private readonly ConcurrentDictionary<ulong, IndexedCollection<ReactionRoleMessage>> _models;
/// <summary>
/// Contains the (Message ID, User ID) of reaction roles that are currently being processed.
/// </summary>
private readonly ConcurrentHashSet<(ulong, ulong)> _reacting = new();
public RoleCommandsService(DiscordSocketClient client, DbService db,
Bot bot)
{
@@ -38,75 +45,58 @@ namespace NadekoBot.Modules.Administration.Services
private Task _client_ReactionAdded(Cacheable<IUserMessage, ulong> msg, ISocketMessageChannel chan, SocketReaction reaction)
{
var _ = Task.Run(async () =>
_ = Task.Run(async () =>
{
try
if (!reaction.User.IsSpecified ||
reaction.User.Value.IsBot ||
reaction.User.Value is not SocketGuildUser gusr ||
chan is not SocketGuildChannel gch ||
!_models.TryGetValue(gch.Guild.Id, out var confs))
return;
var conf = confs.FirstOrDefault(x => x.MessageId == msg.Id);
if (conf is null)
return;
// compare emote names for backwards compatibility :facepalm:
var reactionRole = conf.ReactionRoles.FirstOrDefault(x => x.EmoteName == reaction.Emote.Name || x.EmoteName == reaction.Emote.ToString());
if (reactionRole != null)
{
if (!reaction.User.IsSpecified ||
reaction.User.Value.IsBot ||
!(reaction.User.Value is SocketGuildUser gusr))
return;
if (!(chan is SocketGuildChannel gch))
return;
if (!_models.TryGetValue(gch.Guild.Id, out var confs))
return;
var conf = confs.FirstOrDefault(x => x.MessageId == msg.Id);
if (conf is null)
return;
// compare emote names for backwards compatibility :facepalm:
var reactionRole = conf.ReactionRoles.FirstOrDefault(x => x.EmoteName == reaction.Emote.Name || x.EmoteName == reaction.Emote.ToString());
if (reactionRole != null)
if (!conf.Exclusive)
{
if (conf.Exclusive)
{
var roleIds = conf.ReactionRoles.Select(x => x.RoleId)
.Where(x => x != reactionRole.RoleId)
.Select(x => gusr.Guild.GetRole(x))
.Where(x => x != null);
var __ = Task.Run(async () =>
{
try
{
//if the role is exclusive,
// remove all other reactions user added to the message
var dl = await msg.GetOrDownloadAsync().ConfigureAwait(false);
foreach (var r in dl.Reactions)
{
if (r.Key.Name == reaction.Emote.Name)
continue;
try { await dl.RemoveReactionAsync(r.Key, gusr).ConfigureAwait(false); } catch { }
await Task.Delay(100).ConfigureAwait(false);
}
}
catch { }
});
await gusr.RemoveRolesAsync(roleIds).ConfigureAwait(false);
}
var toAdd = gusr.Guild.GetRole(reactionRole.RoleId);
if (toAdd != null && !gusr.Roles.Contains(toAdd))
{
await gusr.AddRolesAsync(new[] { toAdd }).ConfigureAwait(false);
}
await AddReactionRoleAsync(gusr, reactionRole);
return;
}
else
// If same (message, user) are being processed in an exclusive rero, quit
if (!_reacting.Add((msg.Id, reaction.UserId)))
return;
try
{
var dl = await msg.GetOrDownloadAsync().ConfigureAwait(false);
await dl.RemoveReactionAsync(reaction.Emote, dl.Author,
new RequestOptions()
{
RetryMode = RetryMode.RetryRatelimit | RetryMode.Retry502
}).ConfigureAwait(false);
Log.Warning("User {0} is adding unrelated reactions to the reaction roles message.", dl.Author);
var removeExclusiveTask = RemoveExclusiveReactionRoleAsync(msg, gusr, reaction, conf, reactionRole, CancellationToken.None);
var addRoleTask = AddReactionRoleAsync(gusr, reactionRole);
await Task.WhenAll(removeExclusiveTask, addRoleTask).ConfigureAwait(false);
}
finally
{
// Free (message/user) for another exclusive rero
_reacting.TryRemove((msg.Id, reaction.UserId));
}
}
catch { }
else
{
var dl = await msg.GetOrDownloadAsync().ConfigureAwait(false);
await dl.RemoveReactionAsync(reaction.Emote, dl.Author,
new RequestOptions()
{
RetryMode = RetryMode.RetryRatelimit | RetryMode.Retry502
}).ConfigureAwait(false);
Log.Warning("User {0} is adding unrelated reactions to the reaction roles message.", dl.Author);
}
});
return Task.CompletedTask;
@@ -114,16 +104,16 @@ namespace NadekoBot.Modules.Administration.Services
private Task _client_ReactionRemoved(Cacheable<IUserMessage, ulong> msg, ISocketMessageChannel chan, SocketReaction reaction)
{
var _ = Task.Run(async () =>
_ = Task.Run(async () =>
{
try
{
if (!reaction.User.IsSpecified ||
reaction.User.Value.IsBot ||
!(reaction.User.Value is SocketGuildUser gusr))
reaction.User.Value is not SocketGuildUser gusr)
return;
if (!(chan is SocketGuildChannel gch))
if (chan is not SocketGuildChannel gch)
return;
if (!_models.TryGetValue(gch.Guild.Id, out var confs))
@@ -193,5 +183,71 @@ namespace NadekoBot.Modules.Administration.Services
uow.SaveChanges();
}
}
/// <summary>
/// Adds a reaction role to the specified user.
/// </summary>
/// <param name="user">A Discord guild user.</param>
/// <param name="dbRero">The database settings of this reaction role.</param>
private Task AddReactionRoleAsync(SocketGuildUser user, ReactionRole dbRero)
{
var toAdd = user.Guild.GetRole(dbRero.RoleId);
return (toAdd != null && !user.Roles.Contains(toAdd))
? user.AddRoleAsync(toAdd)
: Task.CompletedTask;
}
/// <summary>
/// Removes the exclusive reaction roles and reactions from the specified user.
/// </summary>
/// <param name="reactionMessage">The Discord message that contains the reaction roles.</param>
/// <param name="user">A Discord guild user.</param>
/// <param name="reaction">The Discord reaction of the user.</param>
/// <param name="dbReroMsg">The database entry of the reaction role message.</param>
/// <param name="dbRero">The database settings of this reaction role.</param>
/// <param name="cToken">A cancellation token to cancel the operation.</param>
/// <exception cref="OperationCanceledException">Occurs when the operation is cancelled before it began.</exception>
/// <exception cref="TaskCanceledException">Occurs when the operation is cancelled while it's still executing.</exception>
private Task RemoveExclusiveReactionRoleAsync(Cacheable<IUserMessage, ulong> reactionMessage, SocketGuildUser user, SocketReaction reaction, ReactionRoleMessage dbReroMsg, ReactionRole dbRero, CancellationToken cToken = default)
{
cToken.ThrowIfCancellationRequested();
var roleIds = dbReroMsg.ReactionRoles.Select(x => x.RoleId)
.Where(x => x != dbRero.RoleId)
.Select(x => user.Guild.GetRole(x))
.Where(x => x != null);
var removeReactionsTask = RemoveOldReactionsAsync(reactionMessage, user, reaction, cToken);
var removeRolesTask = user.RemoveRolesAsync(roleIds);
return Task.WhenAll(removeReactionsTask, removeRolesTask);
}
/// <summary>
/// Removes old reactions from an exclusive reaction role.
/// </summary>
/// <param name="reactionMessage">The Discord message that contains the reaction roles.</param>
/// <param name="user">A Discord guild user.</param>
/// <param name="reaction">The Discord reaction of the user.</param>
/// <param name="cToken">A cancellation token to cancel the operation.</param>
/// <exception cref="OperationCanceledException">Occurs when the operation is cancelled before it began.</exception>
/// <exception cref="TaskCanceledException">Occurs when the operation is cancelled while it's still executing.</exception>
private async Task RemoveOldReactionsAsync(Cacheable<IUserMessage, ulong> reactionMessage, SocketGuildUser user, SocketReaction reaction, CancellationToken cToken = default)
{
cToken.ThrowIfCancellationRequested();
//if the role is exclusive,
// remove all other reactions user added to the message
var dl = await reactionMessage.GetOrDownloadAsync().ConfigureAwait(false);
foreach (var r in dl.Reactions)
{
if (r.Key.Name == reaction.Emote.Name)
continue;
try { await dl.RemoveReactionAsync(r.Key, user).ConfigureAwait(false); } catch { }
await Task.Delay(100, cToken).ConfigureAwait(false);
}
}
}
}