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

WIP: Fetch channel history at startup #376

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

Conversation

dlmarquis
Copy link
Collaborator

This PR is a bit all over the place, and that's my fault for wandering off into side projects without making a separate branch like a good boy. As it is now, this branch is in a much saner state than it was this morning, but let me know if you want me to cut it up into smaller chunks.

The biggest change is some rudimentary ratelimiting that allows us to finally query missed group chat messages at startup. Experimentally, Discord has a 5-per-5-second limit on any given endpoint group (per channel, per guild, per webhook). The only place we run into that enough to cause serious problems is history fetching, so I added a 1-second delay before it fetches the next batch of messages. I also added a 30ms delay before any http fetch towards the end of not hitting the global ratelimit. The fetch process should also now abort and resend the request if it receives a code 429 ratelimit exceeded in the response header.

To make the history fetching experience a little better, I added channel settings that allow you to override whether a single channel is treated as large or small.

I also updated some enums, and added message type and permission flag enums to make the code a little less obtuse.

Outside of that, there are a few stray fixes to errors I've made in previous PRs.

Danielle added 3 commits October 29, 2021 16:45
libdiscord.c Outdated
guint64 retry_after = retry_after_s ? to_int(retry_after_s) : 5;
PurpleHttpRequest *request = purple_http_conn_get_request(http_conn);

sleep(retry_after);
Copy link
Owner

Choose a reason for hiding this comment

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

Sleep() will lock up the whole UI, and block other prpls - it can cause disconnections and drops of TCP connections too. It'll need to be switched over to a non-blocking purple_set_timeout() call

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah, I had a sneaking suspicion sleep was the wrong function here. Unfortunately all the examples I looked at were for threaded applications. I didn't know about the libpurple solution, thanks!

libdiscord.c Outdated
ts.tv_sec = nap / 1000;
ts.tv_nsec = (nap % 1000) * 1000000;
nanosleep(&ts, NULL);
#endif
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, sorry, these sleeps are really bad in single-threaded applications like libpurple/Pidgin ;(

libdiscord.c Outdated
@@ -5765,6 +5934,8 @@ discord_got_history_of_room(DiscordAccount *da, JsonNode *node, gpointer user_da
if (rolling_last_message_id < last_message) {
/* Request the next 100 messages */
gchar *url = g_strdup_printf("https://" DISCORD_API_SERVER "/api/" DISCORD_API_VERSION "/channels/%" G_GUINT64_FORMAT "/messages?limit=100&after=%" G_GUINT64_FORMAT, channel->id, rolling_last_message_id);

sleep(1);
Copy link
Owner

Choose a reason for hiding this comment

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

:(

@@ -5982,6 +6198,20 @@ discord_got_channel_info(DiscordAccount *da, JsonNode *node, gpointer user_data)
purple_chat_conversation_set_topic(chatconv, NULL, json_object_get_string_member(channel, "name"));
}

if (json_object_has_member(channel, "last_pin_timestamp")) {
guint64 last_message_id = discord_get_room_last_id(da, int_id);
guint64 last_message_time = ((last_message_id >> 22) + 1420070400000)/1000;
Copy link
Owner

Choose a reason for hiding this comment

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

What's the magic number for?

Copy link

Choose a reason for hiding this comment

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

That's for converting Discord snowflake to Unix timestamp.

Not sure why the second variable is a u64 and not time_t.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's u64 because I was using millisecond precision originally and I just never changed it 🤦. I'm likely to use the same code in my other PR, so it might make sense to make a small function for converting a Discord snowflake to a timestamp?

@EionRobb
Copy link
Owner

Love all the enums and clean up. Thank you!

Sadly the sleeps will cause too many other problems :(

@dlmarquis dlmarquis changed the title Rudimentary ratelimiting, fetch channel history at startup, some QoL tweaks WIP: Rudimentary ratelimiting, fetch channel history at startup, some QoL tweaks Nov 4, 2021
@dlmarquis dlmarquis changed the title WIP: Rudimentary ratelimiting, fetch channel history at startup, some QoL tweaks Rudimentary ratelimiting, fetch channel history at startup, some QoL tweaks Nov 12, 2021
@dlmarquis
Copy link
Collaborator Author

dlmarquis commented Nov 12, 2021

Okay, I think I've addressed the issues now. The sucky part is I had to defer processing the READY statement to include info from READY_SUPPLEMENTAL to get the channel size handling correct, and I'm fairly certain I've handled that poorly. I'd love to brainstorm a better way to get at this.

At this point I think it makes sense to separate out the history grabbing at start from the rest. I'll make a new pull request in a moment.

@dlmarquis dlmarquis changed the title Rudimentary ratelimiting, fetch channel history at startup, some QoL tweaks WIP: Fetch channel history at startup Nov 12, 2021
@EionRobb
Copy link
Owner

Okay, I think I've addressed the issues now. The sucky part is I had to defer processing the READY statement to include info from READY_SUPPLEMENTAL to get the channel size handling correct, and I'm fairly certain I've handled that poorly. I'd love to brainstorm a better way to get at this.

This also assumes that READY_SUPPLEMENTAL always gets triggered, which it's not guaranteed to

@dlmarquis
Copy link
Collaborator Author

dlmarquis commented Nov 16, 2021

This also assumes that READY_SUPPLEMENTAL always gets triggered, which it's not guaranteed to

Does it not? I checked a few other projects using the user api like DISCUM, and they all assume READY_SUPPLEMENTAL gets sent every time. Do you know what conditions cause it to not get triggered?

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