Discord.py: Signal handling in run() prevents threading

[ghost]

Signal handling (for non-win32 platforms) should atleast be made optional, in case the users don't want to run their bot instance in the main thread or use their own signal handling.

Perhaps might have missed an option myself and there is another proper workaround (and I apologize if so), but as it stands the easiest way I've found to work around this (when updating a bot to use the rewrite branch) was to comment it out.

Traceback (most recent call last):
  File "/usr/lib/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.6/threading.py", line 864, in run
    self._target(*self._args, **self._kwargs)
  File "main.py", line 85, in connect_discord
    discordbot.run(token)
  File "/home/*******/.local/lib/python3.6/site-packages/discord/client.py", line 553, in run
    loop.add_signal_handler(signal.SIGINT, self._do_cleanup)
  File "/usr/lib/python3.6/asyncio/unix_events.py", line 102, in add_signal_handler
    raise RuntimeError(str(exc))
RuntimeError: set_wakeup_fd only works in main thread

[Ikusaba-san]

What are you doing that requires client.run() to be run on a separate thread? Usually it never needs to be on a separate thread unless you're doing something dubious.

[ghost]

Basically, running bots for different platforms together and leaving the main thread open for other operations. Also, for example, some actions/initializations require waiting on the different clients to be ready and I'm using threading events for that purpose. One thing I'm actually wondering though, is why a library is calling (or rather, _enforcing_) signals in the first place; it should atleast provide with an option to disable those in my opinion.

[bmeh]

I concur. Firstly, signal handlers such as these have no place in a library, and libraries shouldn't even exit on their own, they should return an error (to the user of the library), throw an exception, or anything that would make the user responsible for handling those errors, and secondly, one of the top priorities of a library should be eliminating those error handling paths to begin with, not adding them without giving control to the user. It's not the job of a library to handle errors by exiting or handling anything at the expense of reducing the control of any user of a library, at least in the case of error handling.

[Rapptz]

@Theriaca If you want to gain more control over the loop, you are free to use the lower-level primitives yourself. Client.run is just meant as a user-friendly wrapper for Client.login + Client.connect.

Indeed, a cursory look at the documentation for Client.run even details this:

If you want more control over the event loop then this function should not be used. Use start() coroutine or connect() + login().

While also giving a short simple example on how to do some of the handling. The complications with clean-up are there to minimise bug reports from improper clean-up from the library itself.

If you want to launch something on another thread using asyncio (after all, these are coroutines) you would have to use asyncio.run_coroutine_threadsafe and work from there.

@bmeh

As seen above, it doesn't dictate your usage and it has the option to not be used at all. It's an option that is easy to use for the 99% of use cases with very little harm in addition. Now, had you actually seen a library that was in use with many people you would soon realise that doing something like this isn't entirely unheard of. Indeed, abstracting something to the point of being easy to use at the expense of some form of degradation (such as performance or resource control) is seen in quite a few popular libraries:

1. requests -- Despite being less performant to do so, they abstract Session creation for ease-of-use, but you do not lose the ability to create Session objects.
2. aiohttp -- Allows simple running of a webserver that does its own and similar clean-up.

I could list more examples but this is hopefully enough. I appreciate your opinion, but unfortunately it's irrelevant to this issue.

[bmeh]

Except you are adding unnecessary inconsistency between platforms for no reason whatsoever other than "I like it that way". If you are not willing to make it optional - which is quite easy to do so -, then expect your library to be put aside by many people (no, I'm not going to pull percentages out of my a$$). I'm sorry you could not be convinced to implement something so simple, but judging from your latest commit history, and the decent amount of unanswered pull requests, it seems to me that you have no interest in maintaining it anymore.

Your claim of what I have said being irrelevant is just outright false. On what grounds did you conclude that it adds "very little harm", and what option are you talking about if you are not even convinced to add an optional bool variable? It's apparently an issue, otherwise this wouldn't have been reported, but I guess not many people have reported it. Is that why you think it's irrelevant?

I mean you are right about not using it at all, after all, no one actually has to use a library that doesn't aim to be a library, but aims to be an actual, full blown client with its own error handling, and foolish inconsistency between various platforms, and so on.

At this point it shouldn't even be considered an "API wrapper", because it certainly is NOT an API wrapper, it's much more than that, with lots of unnecessary side-effects, etc.

[Rapptz]

Except you are adding unnecessary inconsistency between platforms for no reason whatsoever other than "I like it that way".

It's because signal handlers are not available on Windows. Nothing to do with my personal opinion on any platform.

If you are not willing to make it optional - which is quite easy to do so -, then expect your library to be put aside by many people (no, I'm not going to pull percentages out of my a$$).

There are workarounds that were designed explicitly with this use case in mind.

I'm sorry you could not be convinced to implement something so simple, but judging from your latest commit history, and the decent amount of unanswered pull requests, it seems to me that you have no interest in maintaining it anymore.

You have no investment in this project nor do you have any right to make any comment on the investment I personally have in this project.

Your claim of what I have said being irrelevant is just outright false.

It's not false. Your opinion is just that, an opinion. It doesn't solve the issue at hand nor does it help the author with the original problem. It's just noise. Just like the rest of this reply.

[ghost]

Indeed, abstracting something to the point of being easy to use at the expense of some form of degradation (such as performance or resource control) is seen in quite a few popular libraries

If you take a look at many popular libraries, (for instance, SDL) you'll notice a lot include signal handling, and in the case of SDL and many others, they also include a way to turn it off; looking at the library code, you would only have to possibly pass an additional argument (e.g. handle_signal) and check against it before handling.

Additionally, this issue brings up something else: there _is_ a notable inconsistency between running the same exact code on different platforms: You can use run() in its own thread just fine on Windows since there are no signals supported for that platform, but the moment you'll switch to e.g. Linux, it'll blow up in your face.

This is an excellent opportunity to both allow your users to have more control on the function (you can always have it handle by default when nothing is passed), and make the system more consistent across platforms (that is, if you actually consider there are better options and feel like implementing them).

And all things considered, it would in no way degrade usability.

[bmeh]

Nothing to do with my personal opinion on any platform.

You still decided to add those signal handlers for everything that's not Windows, and that was your decision to do so. You didn't have to ruin something that could have been handled otherwise by the user of the library.

There are workarounds that were designed explicitly with this use case in mind.

Yeah, and you know what would be a better solution to resolve this particular issue? Adding two lines of code, and making it optional. You could default to it if you'd like unless specified otherwise.

You have no investment in this project nor do you have any right to make any comment on the investment I personally have in this project.

I don't need to be invested in a project to conclude you have no interest in this project. Of course I cannot read your mind, nor your intentions, but I can clearly judge it based on your recent commit history, and the amount of pull requests that have been ignored, along with your handling of this particular issue which is a simple fix.

It's not false. Your opinion is just that, an opinion. It doesn't solve the issue at hand nor does it help the author with the original problem. It's just noise. Just like the rest of this reply.

I've already given you a solution, but let me ask you: do you want me to write the code for you (2-4 lines at most) and send a pull request just so it can be ignored along with the rest?

[Rapptz]

If you take a look at many popular libraries, (for instance, SDL) you'll notice a lot include signal handling, and in the case of SDL and many others, they also include a way to turn it off; looking at the library code, you would only have to possibly pass an additional argument (e.g. handle_signal) and check against it before handling.

This isn't in the same vain as the things I've mentioned.

Additionally, this issue brings up something else: there is a notable inconsistency between running the same exact code on different platforms: You can use run() in its own thread just fine on Windows since there are no signals supported for that platform, but the moment you'll switch to e.g. Linux, it'll blow up in your face.

You can't run Client.run in another thread on Windows with well defined behaviour anyway. Since it's using coroutines you have to be careful about multi-threading on asyncio and it will not work as expected unless you take some special handling on how the event loop is handled. i.e. you would need to be cautious on spawning the thread and the loop interaction. Something correct would be also abstracting the creation of the Client in the thread's function.

This is an excellent opportunity to both allow your users to have more control on the function (you can always have it handle by default when nothing is passed), and make the system more consistent across platforms (that is, if you actually consider there are better options and feel like implementing them).

Once again I will re-iterate, I do not intend these use-cases to be provided by Client.run because it's not the proper abstraction for it. Client.run is purposefully aggressive. It handles clean-up and it takes control of the loop. These things are undesirable on something that requires more fine grained control so there are already tools to recreate the function itself. It's not worth doing extra leg-work to allow people to bypass certain clean-up mechanisms when they can just have the more fine-grained control they ask for with separate functions.

[d-Rickyy-b]

Additionally, this issue brings up something else: there _is_ a notable inconsistency between running the same exact code on different platforms: You can use run() in its own thread just fine on Windows since there are no signals supported for that platform, but the moment you'll switch to e.g. Linux, it'll blow up in your face.

Exactly what has happened to me just today. I developed my whole code on Windows, started deploying it to my VPS and boom nothing works, errors all over the place. It's sad to see that the maintainers won't add such an easy to implement switch.

Regarding:

Usually it never needs to be on a separate thread unless you're doing something dubious.

I am standing exactly in front of the same issue as the original reporter: Connecting multiple platforms (in my case Telegram & Discord) in one application, so that you can get Telegram notifications when someone joins a discord channel. However this issue makes it pretty hard to implement it.