Unexpected behaviour due to non atomic API calls

[almostSouji]

Some library methods can result in unexpected behavior based on undocumented internal calls and API behavior. There surely are points to be made for documentation to prevent confusing library users. However, I'm presently not too sure if this can be feasibly done without the documentation turning into confusing clutter.

One example of such a method is GuildMemberRoleManger#add (and accordingly remove). Provided with multiple roles this method internally calls GuildMemberRoleManager#set

discord.js/src/managers/GuildMemberRoleManager.js

Lines 88 to 89 in 8a7abc9

	const newRoles = [...new Set(roleOrRoles.concat(...this._roles.values()))];
	return this.set(newRoles, reason);

Which in itself calls GuildMember#edit

discord.js/src/managers/GuildMemberRoleManager.js

Lines 150 to 152 in 8a7abc9

	set(roles, reason) {
	return this.member.edit({ roles }, reason);
	}

Which does a PATCH api call at /guilds/{guild.id}/members/{user.id}

This call now is not atomic. If discord receives multiple patch calls to a specific member in a short timeframe (either by using #remove and #add back to back without sequencing the calls and handling the returned Promises accordingly - or even by different bots having the job to set or restore roles when a member joins the guild).

The API's principle of eventual consistency can here cause unwanted outcomes, making it appear as if member updates were disregarded.

@Wittiest has brought up that discord.py solves this by allowing an option to do atomic updates instead. (link). This flag causes the provided roles to instead be iterated and n calls to the PUT /guilds/{guild.id}/members/{user.id}/roles/{role.id} endpoint to be made.

I personally am unsure if we should go this route, making multiple API calls where the user might not expect them, leading to the end-user being unaware of depleting rate-limits much quicker than expected, possibly causing major unexpected delays in the program flow.

---

Original point made by @Wittiest:

Yeah that's a hard one. I think this sort of thing only makes sense when the default is set to atomic=false so that (hopefully) a user would only opt in after reading and fully comprehending the documentation.

The main benefit here would be that Discord.js would be adding a strongly-consistent bulk add / remove which would be useful for the case you pointed out earlier about multiple bots or the case of a user simultaneously modifying a role. The side benefit would be implicitly documenting the API with that param [...]

[Wittiest]

I personally am unsure if we should go this route, making multiple API calls where the user might not expect them, leading to the end-user being unaware of depleting rate-limits much quicker than expected, possibly causing major unexpected delays in the program flow.

This is definitely a valid point. It looks like the limit on requests to /guilds/{guild.id}/members/{user.id}/roles/{role.id} is 10 TPS per guild for each bot (atm, I know these are dynamic). I can imagine that in a large server a bot could definitely exhaust this, especially since discord applies this single rate limit to both the PUT and DELETE http verbs.

As the atomic flag would default to false, I think we could trust that readers who are using it would read the documentation which could explicitly highlight that this would cause n api calls.

[almostSouji]

I think we could trust that readers who are using it would read the documentation which could explicitly highlight that this would cause n api calls.

Then the question is if a user at that level of proficiency (or maybe even before that) can't be asked to just iterate the desired roles and call add and remove multiple times, according to their use case