-
Notifications
You must be signed in to change notification settings - Fork 30
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
Segfault due to race conditions in discord_cleanup when called from another thread #135
Comments
Your suggested solution makes sense,
Yeah, this ordering will lead to issues as we need to ensure The workflow is:
So discord_shutdown() is asynchronous, all it does is notify concord that it SHOULD shut down sometime in the near future... We'll only know for sure the bot has shutdown once |
Thanks for confirming! A couple other clarifications from trying to figure these out the past couple days, and I can certainly open a new issue if this is a legitimate issue, not just user error, but I wonder if these are other issues that may exist: 2 things I'm struggling to do:
Luckily, both of these only need to be done once. I can get presence updates just fine, but not actually the initial presences when the module starts, so I only accumulate presences over time, but don't have the initial presences. It seems this should be possible, but all the APIs either return
I did notice some other events seem to return NULL for certain fields that other functions do include, so I thought maybe this was for efficiency reasons (don't want to fetch more than we need to). This suggests guild member would be the right API to use, since it has presence: https://discord.js.org/#/docs/main/stable/class/GuildMember But, according to the documentation for this, there is no presence field, even though it would seem this would be the logical place for it: https://cogmasters.github.io/concord/structdiscord__guild__member.html There is also this one: https://discord.com/developers/docs/resources/guild#guild-widget-object But, looking at what the response contains, I can't find any fields that actually contain any presence data. Certainly, I've scoured both the code and the documentation, but have come up short on other ways to get a guild member's presence on demand, which seems necessary if there's no way to get it from
The assertion fails, because the member field is always Yes, the relevant intents should be set: And all of those are toggled on the bot side as well, so I don't think it's permissions. Any thoughts on this? Are these even the right APIs to use? |
@InterLinked1 I can take a look at those separate issues later on, in the meantime can you confirm that the JSON payloads sent by Discord do in fact contain the field you're looking for? Please ensure that:
On another note, I see that you are using djs as a reference as to what to expect from the Concord's API, instead may I suggest you take a look at the official Discord API? Our goal is to keep concord a 1:1 mapping to the official API, so if you spot something there and seems to be missing from Concord chances are we need to have it be updated. According to the Discord API, there is no presence field for guild members, which means djs must fetch that internally in some other way. |
From what I can see, yes, it does appear the presences are getting sent, but either I'm not using the library right or the library isn't exposing these: This is the beginning of a line that contains a whole JSON array of presence data, which appears after a request to
The fact that a whole array of presences is coming through leads me to believe my first snippet is probably the right idea, but for some reason it's just NULL for me. |
fix serializing for 'struct discord_request_guild_members' Part of #135
There was indeed an issue with 'struct discord_request_guild_members', you can get the latest fix from the I used the following snippet while working on a fix: #include <stdio.h>
#include <stdlib.h>
#include <concord/discord.h>
#include <concord/log.h>
void
on_members_chunk(struct discord *client, const struct discord_guild_members_chunk *event)
{
const int n = event->presences ? event->presences->size : 0;
char text[8192];
for (int i = 0; i < n; ++i) {
discord_presence_update_to_json(text, sizeof(text), event->presences->array + i);
log_info("%s\n", text);
}
}
void
on_members_get(struct discord *client, const struct discord_message *event)
{
if (event->author->bot) return;
struct discord_request_guild_members request = {
.guild_id = event->guild_id,
.presences = true,
};
discord_request_guild_members(client, &request);
}
int main(void)
{
ccord_global_init();
struct discord *client = discord_config_init("config.json");
discord_add_intents(client, DISCORD_GATEWAY_GUILD_PRESENCES
| DISCORD_GATEWAY_GUILD_MEMBERS);
discord_set_on_guild_members_chunk(client, &on_members_chunk);
discord_set_on_command(client, "members_get", &on_members_get);
discord_run(client);
discord_cleanup(client);
ccord_global_cleanup();
}
Now this solves the first issue you encountered, but it doesn't seem |
Okay, so I'm getting some presence info now, but it seems to fail halfway through, for example starting around member 34 of 77 in one test, event->presences->array + i starts pointing to invalid memory, and I get a segfault.
Printing out the sizes, I get an interesting result: Is it expected that |
Do you mind opening this as a separate issue? Also, can you verify by the |
fix serializing for 'struct discord_request_guild_members' Part of #135
fix serializing for 'struct discord_request_guild_members' Part of #135
Describe the bug
I'm using
libdiscord
in a shared library module that I built for a large multithreaded application, e.g. Concord is only a very small part of the entire program, and there are lots of other threads. Everything is pretty much working, but I'm having segmentation faults on shut down, and it appears it may be because I'm shutting down the client in one thread, and perhaps this isn't legal. I looked at some of the code and it appears ifhave_sigint
is set, thendiscord_run
will exit. I'm not using SIGINT because that only makes sense for a standalone program, but perhaps this suggests that there's no way to terminate a session "out of band" (apart from this mechanism)? Is this the right understanding?Essentially, I have this (some stuff removed for conciseness):
I initially had this ordering:
When I did this, I got segmentation faults in the thread with
discord_run
, suggesting that perhaps it is not legal to calldiscord_shutdown
in another thread. However, I need to be able to do that... how else can I tell the client to exit?With the ordering shown in the larger snippet,
pthread_join
never returns, so the unload blocks forever (though no crash).Is there an example or suggested approaching for doing an out of band shutdown as I'm trying to do here? I'd think this would be a common use case, but I didn't find any examples of it. It seems like there is a race condition here with the way I'm currently doing it that leads to the
discord_run
thread trying to read invalid memory... and if I don't calldiscord_shutdown
, then thediscord_run
thread won't exit, so I'm not sure if there's a way around that.Perhaps something like the following could be added to
src/concord-once.c
? (Although I'm not positive if this would be the right fix or not.)My thinking is this would allow the discord_thread to gracefully exit on its own, and then I could call
pthread_join
directly, since thediscord_run
thread itself appears to calldiscord_shutdown
on its own. So really, maybe what's need is just a way to asynchronously shut down using a public function, generically, not just the internal SIGINT handler which doesn't apply to every use case. (Correct me if I'm wrong, but I don't think this currently exists from what I could find.)This happens consistently if I shut down my program (which unloads all dynamic modules), but not always (only sometimes) if I only attempt to unload the Discord module:
Stack Trace for full shutdown
Stack Trace for explicit unload
The text was updated successfully, but these errors were encountered: