Closed albertz closed 6 years ago
Hmm; I don't think I quite understand. What do you mean by "the original behavior" here? Are you saying that GLib itself (or perhaps GStreamer, or perhaps PyGObject—not sure which) installs a SIGINT handler when it's loaded? If the latter, perhaps this bug is best addressed in one of those projects instead of audioread itself?
Sorry, bad formulation. I edited it. PyGObject installs a SIGINT handler, and the main Python thread will never get a KeyboardInterrupt, which is not what you want. You can see that in the code. Actually, not in the upstream code on GitHub (here), but in the Ubuntu 16 version:
class MainLoop(GLib.MainLoop):
# Backwards compatible constructor API
def __new__(cls, context=None):
return GLib.MainLoop.new(context, False)
# Retain classic pygobject behaviour of quitting main loops on SIGINT
def __init__(self, context=None):
def _handler(loop):
loop.quit()
loop._quit_by_sigint = True
# We handle signal deletion in __del__, return True so GLib
# doesn't do the deletion for us.
return True
if sys.platform != 'win32':
# compatibility shim, keep around until we depend on glib 2.36
if hasattr(GLib, 'unix_signal_add'):
fn = GLib.unix_signal_add
else:
fn = GLib.unix_signal_add_full
self._signal_source = fn(GLib.PRIORITY_DEFAULT, signal.SIGINT, _handler, self)
def __del__(self):
if hasattr(self, '_signal_source'):
GLib.source_remove(self._signal_source)
def run(self):
super(MainLoop, self).run()
if hasattr(self, '_quit_by_sigint'):
# caught by _main_loop_sigint_handler()
raise KeyboardInterrupt
pip installing a new version also is somehow broken on Ubuntu. I didn't investigated this further.
Ah, I see! So perhaps the right thing to do would be to add a KeyboardInterrupt
handler around the self.loop.run()
call in MainLoopThread
and explicitly signal the main thread to stop? That would be a little hairy…
Perhaps this SO answer points to a reasonable alternative? https://stackoverflow.com/a/16486080/39182
Yes, that would be another solution. You can use thread.interrupt_main()
to do that.
The SO solution is not so nice as it would just kill your Python program (I think) and not do the proper handling which you might want for KeyboardInterrupt. Actually, calling audioread should just not change the behavior of that in any way. E.g. consider that the user had installed an own custom handler, which would just get overwritten by this.
fyi, pygobject 3.28+ will no longer install a signal handler there if one was already set before and will no longer change anything when not run in the main thread.
There is an easier fix to work around all this:
instead of GLib.MainLoop()
do GLib.MainLoop.new(None, False)
in audioread
Awesome, @lazka; thank you for the background.
Do you happen to know where in the docs I can learn about the difference between the MainLoop()
constructor and the MainLoop.new(None, False)
call? These docs, for example, mention the new
classmethod, which clearly corresponds to the underlying g_main_loop_new function, but it's not clear what using the constructor was doing.
Do you happen to know where in the docs I can learn about the difference between the MainLoop() constructor and the MainLoop.new(None, False) call? These docs, for example, mention the new classmethod, which clearly corresponds to the underlying g_main_loop_new function, but it's not clear what using the constructor was doing.
Sadly nowhere. __init__
was the (buggy) code @albertz posted above before, now it does nothing and the improved logic is in run() instead. new() returns an already initialized instance (from C) and skips __init__
.
Got it; thanks again for the background about how this happened!
GLib.MainLoop.__init__
installs a SIGINT handler which callsGLib.MainLoop.quit()
, and then reraise a KeyboardInterrupt in that thread. However, the main thread will not notice that, and continues whatever it is doing, which mostly means that it will hang as it is waiting for something. What you would want is a KeyboardInterrupt in the main thread, as this is the original behavior when you press Ctrl+C.I came up with this workaround / ugly monkey patch to just disable the SIGINT handler. However, a better fix would be in PyGObject to raise the KeyboardInterrupt in the main thread.