OpenPrinting / system-config-printer

Graphical user interface for CUPS administration
GNU General Public License v2.0
164 stars 90 forks source link

Freeze trying to change the driver #179

Closed LAfricain closed 3 years ago

LAfricain commented 4 years ago

On Ubuntu 20.04 if I try to change the driver of a printer in the settings, system-config-printer freeze on the "looking for a driver" windows.

zdohnal commented 3 years ago

Hi,

sorry for not responding on this - I'm able to reproduce this issue, but that's where I got. My suspicion is bad usage of GTK threads, but I'm a beginner regarding GTK. If anyone is willing to help, I welcome any patch fixing the issue.

In the meantime, you change the driver via f.e. CUPS web ui.

NicoHood commented 3 years ago

I got the same issue.

Edit: I could verify that this does also happen with 1.5.12 but not with 1.5.11.

The diff is quite small, so anything inside those changes must have introduced this bug: https://github.com/OpenPrinting/system-config-printer/compare/1.5.11...1.5.12

Here is a logfile only for opening the driver change dialog using --debug option. I compared it with meld, and they are idential, only 1.5.12 stops at a specific location. The log is from 1.5.11, I've marked the location where 1.5.12 would stop:

https://gist.github.com/NicoHood/1c9e34e9435cfbf05d8860a99a072494

Bugreport for archlinux: https://bugs.archlinux.org/index.php?do=details&action=details.addvote&task_id=69034

NicoHood commented 3 years ago

The following commit introduced the issue. Reverting it fixes the issue. However I cannot tell why the change was made, I only did a try-error to find the commit that introduced the issue. https://github.com/OpenPrinting/system-config-printer/commit/8a623c2f038dd85da3df647531c6a881d235523c

I found that if only one of both statements is used the code will not crash. I guess this is a race condition where the first require call will block the second.

BTW: I also found this require statement in multiple places. I am not sure if the require itself causes the issue or a race condition or something else: https://github.com/OpenPrinting/system-config-printer/search?q=require_version

NicoHood commented 3 years ago

Ping @twaugh @zdohnal

zdohnal commented 3 years ago

Hi @NicoHood!

Sorry, the first day 'at the office' after vacation.

Thank you for the investigation! So it confirms it is connected to GTK 3, because this commit sets GTK version (seems GTK 2 is still the default somewhere, if it works after removing the lines from the commit).

So there are two options:

  1. switch back to gtk2, which will be deprecated in the future and introduce other errors...
  2. refactor the code to comply with gtk3, which should fix the issue, but I'm not experienced enough with GTK to do it yet.

I'm open for PR for the issue, but since the issue has a workaround (deleting and creating a print queue a new, or via CLI), so IMO it is not a pressing issue.

NicoHood commented 3 years ago

Is gtk2 really still needed? I dont think so, as we are not depending on it on Arch Linux. But I dont really know what this function does. For me this is a really annoying issue to be honest. Who can we ask for assistance?

Edit: Adding a new printer results in an internal server error. I am not sure if that is related to this bug, but... uufff

zdohnal commented 3 years ago

Aha, so it is somehow triggered by requiring version 3.0 of Gdk (which comes from Gtk)... however, if the requiring command is removed and I issue an command in python3:

>>> Gdk
<GdkProxyModule <IntrospectionModule 'Gdk' from '/usr/lib64/girepository-1.0/Gdk-3.0.typelib'>>

so Gdk-3.0 is still used (at least on Fedora 32) even without defining the version and this one works... it is really strange...

I'll remove the version requirement which works for now, but it will trigger a warning. The permanent fix would be to refactor s-c-p code where threads are used to reflect GTK 3...

NicoHood commented 3 years ago

Thanks! Would you mind tagging a new bugfix release? Please also consider using GPG signatures #200

bigon commented 3 years ago

I'm not sure this is a good idea to remove the required version.

If for whatever reason Gtk 4.0 bindings are installed in addition to Gtk 3.0 you might end-up with:

from gi.repository import Gdk
gi.require_version('Gtk', '3.0')
from gi.repository import Gtk

RepositoryError: Requiring namespace 'Gdk' version '3.0', but '4.0' is already loaded

Not sure that with the current s-c-p code it would happen, but it's a risk.

But I think I know why it's working with the patch reverted.

If you look at the code, you can see that the gi module is not imported. That means that when gi.require_version('Gdk', '3.0') was called, the rest for the try block was not called because you got an exception.

That means that the code later was also giving an exception and the gdk_threads_enter() and gdk_threads_leave were never called.

Please "revert the revert" and just import gi before the first gi.require_version (can someone with the problem test that? @NicoHood maybe?)

zdohnal commented 3 years ago

(facepalm) totally missed that, thx, it works.

NicoHood commented 3 years ago

Nice!!

zdohnal commented 3 years ago

The fix is in new release - 1.5.15.

tillkamppeter commented 3 years ago

1.5.15 landed in Ubuntu Hisute.