[Bug]: AddRoleAsync() and AddRolesAsync() removes roles after adding them
douglasparker opened this issue · 27 comments
Check The Docs
- I double checked the docs and couldn't find any useful information.
Verify Issue Source
- I verified the issue was caused by Discord.Net.
Check your intents
- I double checked that I have the required intents.
Description
AddRoleAsync() and AddRolesAsync() removes roles after adding them when using Discord.Net 3.10.0.
After downgrading to Discord.Net 3.9.0, AddRoleAsync() and AddRolesAsync() functions as intended.
Version
3.10.0
Working Version
3.9.0
Logs
n/a
Sample
public async Task HandleMenuInteraction(SocketMessageComponent messageComponent)
{
switch(messageComponent.Data.CustomId)
{
case "role-selection":
Console.WriteLine($"{messageComponent.User.Username} has changed their role subscriptions.");
var values = messageComponent.Data.Values.ToList<string>();
Console.WriteLine($"{messageComponent.User.Username} has selected the following role subscriptions:");
foreach(var v in values) {
Console.WriteLine(v);
}
var user = (IGuildUser) messageComponent.User;
if(user != null)
{
List<string> allowedRoles = new List<string>()
{
"Games",
"Technology",
"Programming",
"Art",
"Music"
};
List<ulong> addedRoles = new List<ulong>();
List<ulong> removedRoles = new List<ulong>();
string roleSubscriptions = "";
foreach(var i in allowedRoles) {
try {
var role = user.Guild.Roles.First(x => x.Name == i);
if(values.Contains(role.Name)) {
addedRoles.Add(role.Id);
roleSubscriptions += $@"
:bell: {role.Name}";
Console.WriteLine($"{role.Name} ({role.Id}) was added to {messageComponent.User.Username}.");
}
else {
removedRoles.Add(role.Id);
roleSubscriptions += $@"
:no_entry_sign: {role.Name}";
Console.WriteLine($"{role.Name} ({role.Id}) was removed from {messageComponent.User.Username}.");
}
}
catch(Exception ex) {
Console.WriteLine(ex);
}
}
await user.AddRolesAsync(addedRoles);
await user.RemoveRolesAsync(removedRoles);
await messageComponent.DeferAsync();
await messageComponent.ModifyOriginalResponseAsync(x => x.Content = $"{user.Mention} is now subscribed to the following channels: {roleSubscriptions}");
}
break;
}
}Packages
Discord.Net V3.9.0
I cannot reproduce the issue.
Could you post a sample of the code that has the issue?
@Misha-133 Sure, no problem! I have updated my original report.
Hmmmm
Just've tried your code on 3.10.0 - everything works without any issues
Hmmmm Just've tried your code on 3.10.0 - everything works without any issues
I'm not sure what could be causing this bug. I'm new to using Discord.NET and I am still figuring things out.
I just upgraded the NuGet package to 3.10.0 and the bug appears again.
Downgrading to 3.9.0 results in these functions working as expected according to the Discord.NET documentation.
I imagine there will be more reports as people upgrade.
May be due to #2614
Hmm
Check if it's your bot removing roles in your server's audit logs
This is most likely a race condition error because AddRoles/RemoveRoles modifies the guild user directly and AddRole/RemoveRole does not so if you add role and modify guild user at the same time it will overwrite the role list of the user and ignore the AddRole.
I want to add that both AddRoleAsync and AddRolesAsync have this issue. I do see that Message Components changed with 3.10.0.
Hmm Check if it's your bot removing roles in your server's audit logs
Audit logs do show that the bot is removing roles after they are added.
The thing is, you ( might ) be blocking the gateway thread with those await-s, meaning updated role data isn't available until your whole handler is ran
And the new API changes are based on role data in cache ( I believe )
Instead of
await user.AddRolesAsync(addedRoles);
await user.RemoveRolesAsync(removedRoles);
Try using
await user.ModifyAsync(x => {
x.RoleIds =
});
This will update the user with the roleids given and any not in the list will be removed.
This is most likely a race condition error because AddRoles modifies the guild user directly and AddRole DeleteRole does not so if you add role and modify guild user at the same time it will overwrite the role list of the user.
Ah, yup, that's most likely the issue
@douglasparker You could use user.ModifyAsync instead
in this particular case the code would look like
await user.ModifyAsync(x => x.RoleIds = user.Roles.Select(r => r.Id) // take roles user currently has
.Except(new [] {user.Guild.Id} ) // @everyone role cannot be added or removed
.Except(removedRoles) // remove roles from the user
.Concat(addedRoles) // add roles
.Distinct().ToList());This adds & removes roles in a single API request, so it's more ratelimit friendly.
This is most likely a race condition error because AddRoles modifies the guild user directly and AddRole DeleteRole does not so if you add role and modify guild user at the same time it will overwrite the role list of the user.
Ah, yup, that's most likely the issue
@douglasparker You could use
user.ModifyAsyncinstead in this particular case the code would look likeawait user.ModifyAsync(x => x.RoleIds = user.Roles.Select(r => r.Id) .Except(new [] {user.Guild.Id} ) // @everyone role cannot be added or removed .Except(removedRoles) // remove roles from the user .Concat(addedRoles) // add roles .Distinct().ToList());This adds & removes roles in a single API request, so it's more ratelimit friendly.
I don't want to remove all of the other roles though.
Why is this only an issue in the latest Discord.NET version? This is pretty much the only thing the bot does at the moment. The Gateway thread isn't being blocked for a significant amount of time and there are very few roles being added / removed.
This is most likely a race condition error because AddRoles/RemoveRoles modifies the guild user directly and AddRole/RemoveRole does not so if you add role and modify guild user at the same time it will overwrite the role list of the user and ignore the AddRole.
I've tried using AddRole and AddRoles independently of each other. Not both.
It doesn't matter how long they're blocked for, you won't receive updated data during the entire handler's execution
I don't want to remove all of the other roles though.
It won't remove all roles, it will remove only roles from removedRoles list
I have the same issue, but I figured it happens only when I try to add multiple roles in one call.
I add role A and it is added (checked it on Discord while debugging), then I add role B and it is added it but role A is removed.
If I do it in two different calls then it works fine. It was working with the previous version too and I have not changed any code since the update.
This definitely seems like a bug despite what others have said. It works with the previous version just fine. 🤷
Still sounds like user implementation issue to me. Only way AddRolesAsync would remove a role is if user.roleIds wasn't returning the full list of roles, or was a stale list (as mentioned above regarding gateway blocking the cache update).
@crick3t can you supply your (pruned) code that reproduces this issue?
@cjbonn can you please explain how it works just fine for all previous versions but the latest is breaking? I am not sure how that is a user implementation issue.
Let's say the user have roles, but does not have RoleA and RoleB
SocketGuildUser Context.Guild.GetUser(userID);
var roleA = Context.Guild.Roles.First(x => x.Id == roleAID);
var roleB = Context.Guild.Roles.First(x => x.Id == roleBID);
await (user as IGuildUser).AddRoleAsync(roleA); //After this user has roleA
await (user as IGuildUser).AddRoleAsync(roleB); //After this roleA disappears and user has roleB
The IDs are the ones for the user and roles.
Note that if a user already had any other roles before the call, those will stay. This happens only for the roles added in the same call.
I have also reverted back to 3.9 and the code above is working fine.
@cjbonn just let me know if something is not right or you need more information. Thanks.
@cjbonn can you please explain how it works just fine for all previous versions but the latest is breaking? I am not sure how that is a user implementation issue.
Previous versions implemented AddRolesAsync by locally foreaching over the list and calling AddRoleAsync once for each role. That means if you want to add 5 roles, it would make 5 separate API calls (and accumulate 5 hits against your rate limit).
Discord has an endpoint for bulk-editing roles, and naturally the bulk-edit methods should use that endpoint. But that endpoint sets the user's roles to whatever list is received, rather than adding or removing. Which means AddRolesAsync now locally needs to grab the user's current role list and update it to add any new IDs, then send the full new list to the server.
Take this example:
// local user.RoleIds == [1, 2]; server-side user has roles 1, 2
await user.AddRolesAsync(new[] {3, 4}); // local [1, 2] + [3, 4] sends [1, 2, 3, 4] to server
// local user.RoleIds == [1, 2]; server-side user has roles 1, 2, 3, 4
await user.AddRolesAsync(new[] {5, 6}); // local [1, 2] + [5, 6] == sends [1, 2, 5, 6] to server
// local user.RoleIds == [1, 2]; server-side user has roles 1, 2, 5, 6That first AddRolesAsync will calculate the new role list as [1, 2, 3, 4] and send that to the server. That's fine, the user's roles are now [1, 2, 3, 4]. But the local cache isn't updated until we receive a gateway message from the server telling us about the modified User.
The second AddRolesAsync will calculate the new role list as [1, 2, 5, 6] and send that to the server. The user's roles are now [1, 2, 5, 6]. Because we haven't yet processed the gateway message, so the local cache is stale, so it's not possible to calculate the correct set of roles.
So why aren't we processing the gateway messages in between the two API calls? Well, that depends. If your handler is configured as RunMode.Sync (the default), that'll be one reason: the handler blocks the gateway thread and no gateway messages can be processed until the entire handler is finished. Using RunMode.Async could help, but that can cause several other issues (you can then have multiple handlers run in parallel, so you need to make sure your code has no race conditions. Also the post-execution flow and error handling are completely different, see docs and docs.
But even using RunMode.Async wouldn't definitively solve this: there's no guarantee that you'll receive the gateway message and process it before the next bulk-edit command is run, since multiple async threads are non-deterministically ordered by nature. And without hooking the user updated event there wouldn't be any way to be sure that the cache is updated before the second call.
The simple solution is: don't use multiple bulk (AddRoles/RemoveRoles) role editing methods in a single handler. Do the merge operations yourself and make a single request; Misha provided example code: #2655 (comment). Alternatively do a foreach loop over the roles you want to add/remove and call the singular methods, which is exactly what the old behaviour was - just be aware that this is not ratelimit-friendly.
e: I wonder if there's any reason the bulk-edit methods can't update the local cached copy directly? Maybe it's just hard to do with how it's currently implemented :\
I think it is because the operation is not guaranteed to succeed
I think it is because the operation is not guaranteed to succeed
The api endpoint actually returns the updated user entity, so that could be used to update one in cache (And the ModifyAsync method on rest entities actually does that)
But making it do that on socket entities would be a breaking change at this point
@BobVul That is extremely helpful information, thank you so much for your detailed response. I really appreciate it and I am certain it will help others understand the problem going forward.
@Misha-133 say you await the API call to complete, and assuming you don't block the gateway thread, is it guaranteed for the event to be received before the await is complete? If so, not blocking might help