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

Allow specifying list of user ids in SocketGuild.DownloadUsersAsync #2676

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

compujuckel
Copy link

@compujuckel compujuckel commented Apr 27, 2023

Discord supports specifying a list of user IDs in their Request Guild Members event, but Discord.NET currently does not expose this functionality.

I'm trying to use Discord.NET in a large server (>600k members) and my bot is interested in only a few (100-10,000) of those. Using DownloadUsersAsync to get all users takes very long and also shoots up RAM usage to >2GB

This PR is not really ready to merge (the changes work fine for my use case though), I'm mainly looking to get some feedback on what is needed to get this feature into Discord.NET.

@quinchs
Copy link
Member

quinchs commented Jun 27, 2023

I like this idea. @Misha-133 can you stress test this PR and see what you can find for bugs/bottlenecks?

@Misha-133
Copy link
Member

Misha-133 commented Jun 27, 2023

        await SocketGuild.DownloadUsersAsync(Array.Empty<ulong>());

Supplying an empty collection of users to download results in a deadlock

UPD: awaiting the method in a event handler causes a deadlock on the gateway thread

@Misha-133
Copy link
Member

@quinchs Should DownloadUsersAsync methods block the thread until users are actually downloaded?
This causes deadlock if the method is executed in a event handler

DownloadUsersAsync method that we had before also has this issue. Shouldn't this be at least documented? Or maybe blocking should be tied to a bool parameter

_________
Everything seems to work fine otherwise

@compujuckel
Copy link
Author

Once thing that's currently missing from this PR is this:

Requesting user_ids will continue to be limited to returning 100 members

so this must be chunked when requesting more than 100 members at once


if (data.Nonce.IsSpecified
&& data.ChunkIndex + 1 >= data.ChunkCount
&& _guildMembersRequestTasks.TryRemove(data.Nonce.Value, out var tcs))
Copy link
Member

@Misha-133 Misha-133 Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This being the only way of removing request tasks may result in a memory leak if client misses the event (due to temporary connection loss, discord requesting a reconnect, etc.)
I'm not exactly sure what's the best way around this, but I'd recommend adding a timeout to tasks so it retries/fails the task if the client doesn't receive the member chuck in X amount of time. This would also solve the issue of possible deadlock caused by this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about allowing users to pass a CancellationToken in DownloadUsersAsync(IEnumerable<ulong> userIds)? A fixed timeout seems a bit too inflexible IMHO, since downloading >1k users will actually take a while (=> more than a few seconds). If no CancellationToken is given, we could apply a default timeout

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a default request timeout and a way for users to supply their own cancellation token.

- throw exception when client not connected
- chunk user downloads
Comment on lines +962 to +975

/// <summary>
/// Downloads specific users for this guild with the default request timeout.
/// </summary>
/// <remarks>
/// This method downloads all users specified in <paramref name="userIds" /> through the Gateway and caches them.
/// Consider using <see cref="DownloadUsersAsync(IEnumerable{ulong}, CancellationToken)"/> when downloading a large number of users.
/// </remarks>
/// <param name="userIds">The list of Discord user IDs to download.</param>
/// <returns>
/// A task that represents the asynchronous download operation.
/// </returns>
/// <exception cref="OperationCanceledException">The timeout has elapsed.</exception>
Task DownloadUsersAsync(IEnumerable<ulong> userIds);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of defining 2 methods with the only difference being a cancel token, why not make the token parameter optional?

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.

3 participants