After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 748336 - CUPS blocking calls (httpConnectEncrypt) done in UI thread
CUPS blocking calls (httpConnectEncrypt) done in UI thread
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Printers
3.16.x
Other Linux
: Normal normal
: ---
Assigned To: Felipe Borges
Control-Center Maintainers
Depends on: 755626
Blocks:
 
 
Reported: 2015-04-22 20:59 UTC by benruyl
Modified: 2016-03-10 10:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
printers: port pp_cups_get_dests_async from GSimpleAsyncResult to GTask (3.43 KB, patch)
2015-10-09 14:32 UTC, Felipe Borges
none Details | Review
printers: make cupsGetDests calls async (2.45 KB, patch)
2015-10-09 14:33 UTC, Felipe Borges
none Details | Review
printers: make cups connection test (check status) async (7.84 KB, patch)
2015-10-09 14:33 UTC, Felipe Borges
none Details | Review
printers: don't block on connection test (4.05 KB, patch)
2015-10-09 14:34 UTC, Felipe Borges
none Details | Review
printers: make all cups server connection tests async (19.42 KB, patch)
2015-10-09 14:34 UTC, Felipe Borges
none Details | Review
printers: add pp_cups_get_job_attributes_async (13.26 KB, patch)
2015-10-09 14:34 UTC, Felipe Borges
none Details | Review
screensho of empty printer-panel (31.05 KB, image/png)
2015-10-09 14:47 UTC, Felipe Borges
  Details
printers: make cupsGetDests calls async (5.54 KB, patch)
2016-02-03 13:06 UTC, Felipe Borges
none Details | Review
printers: make cups connection test (check status) async (5.59 KB, patch)
2016-02-03 13:06 UTC, Felipe Borges
none Details | Review
printers: don't block on connection test (4.11 KB, patch)
2016-02-03 13:11 UTC, Felipe Borges
none Details | Review
printers: make all cups server connection tests async (19.18 KB, patch)
2016-02-03 13:43 UTC, Felipe Borges
none Details | Review
printers: add pp_cups_get_job_attributes_async (14.36 KB, patch)
2016-02-03 13:57 UTC, Felipe Borges
none Details | Review
printers: use pp_cups_get_dests_async on cc-printers-panel (2.75 KB, patch)
2016-02-17 14:37 UTC, Felipe Borges
committed Details | Review
printers: do async connection test during launch of panel (5.86 KB, patch)
2016-02-17 14:37 UTC, Felipe Borges
none Details | Review
printers: set current page async (2.60 KB, patch)
2016-02-17 14:38 UTC, Felipe Borges
none Details | Review
printers: renew/cancel cups subscriptions asynchronously (13.07 KB, patch)
2016-02-17 14:38 UTC, Felipe Borges
none Details | Review
printers: get job attributes in a non-blocking manner (async) (11.20 KB, patch)
2016-02-17 14:38 UTC, Felipe Borges
none Details | Review
printers: do async connection test during launch of panel (5.89 KB, patch)
2016-02-23 15:22 UTC, Felipe Borges
none Details | Review
printers: set current page async (2.89 KB, patch)
2016-02-24 16:35 UTC, Felipe Borges
committed Details | Review
printers: renew/cancel cups subscriptions asynchronously (13.06 KB, patch)
2016-02-24 16:35 UTC, Felipe Borges
none Details | Review
printers: get job attributes in a non-blocking manner (async) (10.84 KB, patch)
2016-02-24 16:36 UTC, Felipe Borges
none Details | Review
printers: do async connection test during launch of panel (5.86 KB, patch)
2016-02-25 13:19 UTC, Felipe Borges
committed Details | Review
printers: cancel cups subscriptions asynchronously (6.54 KB, patch)
2016-03-01 11:01 UTC, Felipe Borges
committed Details | Review
printers: renew cups subscriptions asynchronously (12.44 KB, patch)
2016-03-01 12:38 UTC, Felipe Borges
none Details | Review
printers: get printer job attributes async (13.33 KB, patch)
2016-03-03 11:59 UTC, Felipe Borges
none Details | Review
printers: get printer job attributes async (13.43 KB, patch)
2016-03-03 14:47 UTC, Felipe Borges
committed Details | Review
printers: renew cups subscriptions asynchronously (12.44 KB, patch)
2016-03-03 16:51 UTC, Felipe Borges
none Details | Review
printers: renew cups subscriptions asynchronously (15.47 KB, patch)
2016-03-04 09:32 UTC, Felipe Borges
none Details | Review
printers: renew cups subscriptions asynchronously (15.50 KB, patch)
2016-03-04 09:46 UTC, Felipe Borges
none Details | Review
printers: renew cups subscriptions asynchronously (12.74 KB, patch)
2016-03-04 10:41 UTC, Felipe Borges
none Details | Review
printers: renew cups subscriptions asynchronously (14.00 KB, patch)
2016-03-07 11:31 UTC, Felipe Borges
none Details | Review
printers: renew cups subscriptions asynchronously (15.56 KB, patch)
2016-03-09 14:23 UTC, Felipe Borges
committed Details | Review

Description benruyl 2015-04-22 20:59:12 UTC
When lpstat takes a long time to get its data, for example if it will return a bad file descriptor error after more than 30 seconds, the Printers UI freezes completely during this time (empty screen). Such a scenario could happen when a cups server is unreachable (this error could be reproduced by adding ServerName cups.nikhef.nl to /etc/cups/client.conf).

Possible solution: let the lpstat or other querying take place in a different thread.
Comment 1 Bastien Nocera 2015-04-23 12:35:35 UTC
gnome-control-center doesn't call lpstat directly. Could you get a backtrace of the hang when this happens?

This should be possible with:
gdb attach `pidof gnome-control-center`
in a terminal when it happens

and run "thread apply all bt".

Make sure that the debug packages are installed as well.
Comment 2 benruyl 2015-04-23 14:40:55 UTC
I am currently not able to recompile gnome to allow for debugs, but I can provide you with the following output:

Thread 1 (Thread 0x7f98b42859c0 (LWP 2193))

  • #0 poll
    from /usr/lib/libc.so.6
  • #1 httpAddrConnect2
    from /usr/lib/libcups.so.2
  • #2 httpReconnect2
    from /usr/lib/libcups.so.2
  • #3 httpConnect2
    from /usr/lib/libcups.so.2
  • #4 httpConnectEncrypt
    from /usr/lib/libcups.so.2
  • #5 ??
  • #6 ??
  • #7 g_type_create_instance
    from /usr/lib/libgobject-2.0.so.0
  • #8 ??
    from /usr/lib/libgobject-2.0.so.0
  • #9 g_object_new_valist
    from /usr/lib/libgobject-2.0.so.0
  • #10 g_object_new
    from /usr/lib/libgobject-2.0.so.0
  • #11 ??
  • #12 ffi_call_unix64
    from /usr/lib/libffi.so.6
  • #13 ffi_call
    from /usr/lib/libffi.so.6
  • #14 g_cclosure_marshal_generic
    from /usr/lib/libgobject-2.0.so.0
  • #15 g_closure_invoke
    from /usr/lib/libgobject-2.0.so.0
  • #16 ??
    from /usr/lib/libgobject-2.0.so.0
  • #17 g_signal_emit_valist
    from /usr/lib/libgobject-2.0.so.0
  • #18 g_signal_emit
    from /usr/lib/libgobject-2.0.so.0
  • #19 ??
  • #20 g_cclosure_marshal_VOID__BOXEDv
  • #21 ??
    from /usr/lib/libgobject-2.0.so.0
  • #22 g_signal_emit_valist
    from /usr/lib/libgobject-2.0.so.0
  • #23 g_signal_emit
    from /usr/lib/libgobject-2.0.so.0
  • #24 ??
    from /usr/lib/libgtk-3.so.0
  • #25 ??
    from /usr/lib/libgtk-3.so.0
  • #26 ??
    from /usr/lib/libgobject-2.0.so.0
  • #27 g_signal_emit_valist
    from /usr/lib/libgobject-2.0.so.0
  • #28 g_signal_emit
    from /usr/lib/libgobject-2.0.so.0
  • #29 ??
    from /usr/lib/libgtk-3.so.0
  • #30 ??
    from /usr/lib/libgtk-3.so.0
  • #31 gtk_main_do_event
    from /usr/lib/libgtk-3.so.0
  • #32 ??
    from /usr/lib/libgdk-3.so.0
  • #33 g_main_context_dispatch
    from /usr/lib/libglib-2.0.so.0
  • #34 ??
    from /usr/lib/libglib-2.0.so.0
  • #35 g_main_context_iteration
    from /usr/lib/libglib-2.0.so.0
  • #36 g_application_run
    from /usr/lib/libgio-2.0.so.0
  • #37 main


Note that I can successfully reproduce the bug when ServerName cups.nikhef.nl to /etc/cups/client.conf, so maybe you can try this as well.
Comment 3 Bastien Nocera 2015-04-23 14:44:36 UTC
The printers panel is calling httpConnectEncrypt(), which is blocking, all over the place. This needs to be done in a separate thread.

Marek, g_task_run_in_thread() looks like it could help with that.
Comment 4 Felipe Borges 2015-10-09 14:30:40 UTC
I have been working recently on a series of patches to wrap most of the blocking CUPS calls into async operations.

- The httpConnect* functions were mainly used in the printers panel for connection tests. My implementation uses a more native approach with a g_socket_client for this kind of tests.

- The pp_cups_get_job_attributes_async method introduced in the last patch depends of the PpJob object, introduced on bug 755626 (which was not accepted yet; it blocks this one).

With this changes you will be able to have a functional printer panel while loading a CUPS server. I have used the ServerName cups.nikhef.nl suggested by the bug reporter on my experiments.

* TODO:

- We should let users know that we are loading a cups server, instead of just loading the bare empty panel. I think a GNotification saying something like "Loading printer server..." would be nice (Allan?)
Comment 5 Felipe Borges 2015-10-09 14:32:01 UTC
Created attachment 312952 [details] [review]
printers: port pp_cups_get_dests_async from GSimpleAsyncResult to GTask
Comment 6 Felipe Borges 2015-10-09 14:33:19 UTC
Created attachment 312953 [details] [review]
printers: make cupsGetDests calls async
Comment 7 Felipe Borges 2015-10-09 14:33:47 UTC
Created attachment 312954 [details] [review]
printers: make cups connection test (check status) async

use the same approach at the print-notifications plugin at gnome-
settings-daemon: use g_socket_client instead of cups' httpConnect.
Comment 8 Felipe Borges 2015-10-09 14:34:11 UTC
Created attachment 312955 [details] [review]
printers: don't block on connection test

Use async connection test when checking whether a cups server
is reachable.
Comment 9 Felipe Borges 2015-10-09 14:34:36 UTC
Created attachment 312956 [details] [review]
printers: make all cups server connection tests async
Comment 10 Felipe Borges 2015-10-09 14:34:59 UTC
Created attachment 312957 [details] [review]
printers: add pp_cups_get_job_attributes_async
Comment 11 Felipe Borges 2015-10-09 14:47:02 UTC
Created attachment 312958 [details]
screensho of empty printer-panel

Before these changes, the Printer Panel would just open up after the CUPS server was loaded, causing Control Center to freeze. Now it opens empty like in the screenshot attached. That's why I think we have to notify the user somehow.
Comment 12 Felipe Borges 2016-01-19 15:16:44 UTC
Comment on attachment 312952 [details] [review]
printers: port pp_cups_get_dests_async from GSimpleAsyncResult to GTask

Obsoleted by https://bugzilla.gnome.org/show_bug.cgi?id=760581
Comment 13 Marek Kašík 2016-01-27 16:09:01 UTC
Review of attachment 312953 [details] [review]:

Thank you for the patch, I have just 2 minor requests on it.

::: panels/printers/cc-printers-panel.c
@@ +1071,3 @@
+  PpCups                 *cups = (PpCups *) source_object;
+  PpCupsDests            *cups_dests;
+  GError                 *error = NULL;

You are not using the "error" anywhere else so remove its declaration and pass NULL to the finalize function.

@@ +1096,2 @@
   free_dests (self);
+  cups_dests = pp_cups_get_dests_finish (cups, res, &error);

Move this to the beginning of the function please (right after declarations) so it is clear for what we were waiting.
Also, unref the source_object after the _finish() so we don't leak it.
Comment 14 Marek Kašík 2016-01-27 16:17:02 UTC
(In reply to Marek Kašík from comment #13)
> Review of attachment 312953 [details] [review] [review]:
> 
> Thank you for the patch, I have just 2 minor requests on it.
> 
> ::: panels/printers/cc-printers-panel.c
> @@ +1071,3 @@
> +  PpCups                 *cups = (PpCups *) source_object;
> +  PpCupsDests            *cups_dests;
> +  GError                 *error = NULL;
> 
> You are not using the "error" anywhere else so remove its declaration and
> pass NULL to the finalize function.
> 
> @@ +1096,2 @@
>    free_dests (self);
> +  cups_dests = pp_cups_get_dests_finish (cups, res, &error);
> 
> Move this to the beginning of the function please (right after declarations)
> so it is clear for what we were waiting.
> Also, unref the source_object after the _finish() so we don't leak it.

Also move the first 2 hunks from the next patch to this one because they belong there:

@@ -1068,7 +1068,7 @@ actualize_printers_list_cb (GObject      *source_object,
   int                     current_dest = -1;
   int                     i, j;
   int                     num_jobs = 0;
-  PpCups                 *cups = (PpCups *) source_object;
+  PpCups                 *cups = PP_CUPS (source_object);
   PpCupsDests            *cups_dests;
   GError                 *error = NULL;
.
@@ -1097,6 +1097,7 @@ actualize_printers_list_cb (GObject      *source_object,
   cups_dests = pp_cups_get_dests_finish (cups, res, &error);
   priv->dests = cups_dests->dests;
   priv->num_dests = cups_dests->num_of_dests;
+  g_slice_free (PpCupsDests, cups_dests);

Change this to g_free() please.
Comment 15 Marek Kašík 2016-02-02 14:12:02 UTC
Review of attachment 312954 [details] [review]:

::: panels/printers/cc-printers-panel.c
@@ +1069,3 @@
   int                     i, j;
   int                     num_jobs = 0;
+  PpCups                 *cups = PP_CUPS (source_object);

Move this to the previous patch.

@@ +1098,3 @@
   priv->dests = cups_dests->dests;
   priv->num_dests = cups_dests->num_of_dests;
+  g_slice_free (PpCupsDests, cups_dests);

Make it g_free() and move it to the previous patch.

::: panels/printers/pp-cups.h
@@ +70,3 @@
+                                             gpointer              user_data);
+
+gboolean     pp_cups_connection_test_finish (GObject              *source_object,

Make this similar to other finish functions please so that it takes PpCups instead of the source_object. You will probably need to use GTask in the body of the async call.
Comment 16 Felipe Borges 2016-02-03 13:06:35 UTC
Created attachment 320333 [details] [review]
printers: make cupsGetDests calls async
Comment 17 Felipe Borges 2016-02-03 13:06:54 UTC
Created attachment 320334 [details] [review]
printers: make cups connection test (check status) async

use the same approach at the print-notifications plugin at gnome-
settings-daemon: use g_socket_client instead of cups' httpConnect.
Comment 18 Felipe Borges 2016-02-03 13:11:12 UTC
Created attachment 320335 [details] [review]
printers: don't block on connection test

Use async connection test when checking whether a cups server
is reachable.
Comment 19 Felipe Borges 2016-02-03 13:43:16 UTC
Created attachment 320338 [details] [review]
printers: make all cups server connection tests async
Comment 20 Felipe Borges 2016-02-03 13:57:33 UTC
Created attachment 320339 [details] [review]
printers: add pp_cups_get_job_attributes_async
Comment 21 Marek Kašík 2016-02-11 14:34:51 UTC
Review of attachment 320334 [details] [review]:

It looks good. I have just 2 comments on it.

::: panels/printers/pp-cups.c
@@ +104,3 @@
+
+  if (connection != NULL)
+  {

Indent this block by another 2 spaces please.

@@ +109,3 @@
+  }
+
+  g_task_return_pointer (task, result, NULL);

This should be g_task_return_boolean()
Comment 22 Marek Kašík 2016-02-11 15:32:59 UTC
Review of attachment 320335 [details] [review]:

Thank you for the patch. Place the changes from pp-cups.c, pp-utils.c and pp-utils.h to the printers-make-cups-connection-test-check-status-as.patch since I think that they belong there.

::: panels/printers/pp-cups.c
@@ +128,3 @@
 
+  if (is_address_local (address))
+    address = g_strdup_printf ("localhost:%d", port);

You are leaking the address here.
Comment 23 Marek Kašík 2016-02-11 15:39:18 UTC
Review of attachment 320334 [details] [review]:

One more observation.

::: panels/printers/pp-cups.c
@@ +128,3 @@
+
+  client = g_socket_client_new ();
+  g_socket_client_connect_to_host_async (client,

I've noticed something when reviewing the printers-dont-block-on-connection-test.patch. You will need to use the httpConnectEncrypt() (in another thread) since it handles sockets well. Imagine a situation when cups is configured to listen just on a socket and the port 631 is not open. Then this connection test will return that it can not connect even if we can connect to the socket. Btw, I was also thinking whether we shouldn't just return TRUE if the address is a socket but it I guess that it can happen that the socket is not available even if cupsServer() returns its address.
Sorry for pointing you wrong way before.
Comment 24 Marek Kašík 2016-02-11 17:17:55 UTC
Review of attachment 320333 [details] [review]:

Thank you for the patch. We just need one more connection test there.

::: panels/printers/cc-printers-panel.c
@@ +3085,3 @@
                       self);
 
+  priv->cups_status_check_id = g_timeout_add_seconds (CUPS_STATUS_CHECK_INTERVAL,

Add another connection test here and run the timeout only in the case when we can not connect to the CUPS server please. This was the original idea. It was (probably) thought as a back-up for the situation when the calls of populate_printers_list() and attach_to_cups_notifier() couldn't succeed because of missing server.
Comment 25 Marek Kašík 2016-02-12 11:23:57 UTC
Review of attachment 320339 [details] [review]:

I have several comments on this patch. Btw, generate the set of patches with "git format-patch" please or similar to have exact ordering in their names.

::: panels/printers/cc-printers-panel.c
@@ +294,3 @@
     {
+      if ((username = g_variant_lookup_value (attributes, "job-originating-user-name", G_VARIANT_TYPE ("as"))) != NULL &&
+          (printer_uri = g_variant_lookup_value (attributes, "job-printer-uri", G_VARIANT_TYPE ("as"))) != NULL)

You would leak one of those GVariants if only one is returned. This is not probable but still we should handle that case.

@@ +316,3 @@
     }
+
+  g_variant_unref (attributes);

This must be in the "if (attributes != NULL)" block.

::: panels/printers/pp-job.c
@@ +272,3 @@
+  job_uri = g_strdup_printf ("ipp://localhost/jobs/%d", priv->id);
+
+  if (attributes_names)

Check this against NULL please.

@@ +286,3 @@
+                    "requesting-user-name", NULL, cupsUser ());
+      ippAddStrings (request, IPP_TAG_OPERATION, IPP_TAG_KEYWORD,
+                     "requested-attributes", length, NULL, (const char **) requested_attrs);

I think that we should be fine to just pass attributes_names to the ippAddStrings(). I know that get_ipp_attributes_func() does it this way but I was probably too careful.

@@ +298,3 @@
+          attr = ippFindAttribute (response, requested_attrs[j], IPP_TAG_ZERO);
+          n_attrs = ippGetCount (attr);
+          if (attr && n_attrs > 0 && ippGetValueTag (attr) != IPP_TAG_NOVALUE)

Check attr against NULL please.

@@ +301,3 @@
+            {
+              const GVariantType *type = NULL;
+              GVariant           *values[n_attrs];

Is this really possible?

@@ +324,3 @@
+
+                    for (i = 0; i < n_attrs; i++)
+                      values[i] = g_variant_new_string (g_strdup (ippGetString (attr, i, NULL)));

You shouldn't need to duplicate the string here.

@@ +352,3 @@
+
+              g_variant_builder_add (&builder, "{sv}",
+                                     g_strdup (requested_attrs[j]),

You shouldn't need to duplicate the string here.

@@ +375,3 @@
+
+  task = g_task_new (job, cancellable, callback, user_data);
+  g_task_set_task_data (task, g_strdupv (attributes_names), g_free);

You should use g_strfreev() here.

::: panels/printers/pp-job.h
@@ +25,3 @@
 #include <glib-object.h>
+#include <gio/gio.h>
+#include <cups/cups.h>

Move this include to the pp-job.c since you haven't added a code which needs this here.
Comment 26 Marek Kašík 2016-02-16 12:14:36 UTC
Review of attachment 320338 [details] [review]:

Divide this one to more smaller patches. It is hard to do review for it.
Comment 27 Felipe Borges 2016-02-17 14:37:19 UTC
Created attachment 321492 [details] [review]
printers: use pp_cups_get_dests_async on cc-printers-panel
Comment 28 Felipe Borges 2016-02-17 14:37:47 UTC
Created attachment 321493 [details] [review]
printers: do async connection test during launch of panel
Comment 29 Felipe Borges 2016-02-17 14:38:11 UTC
Created attachment 321494 [details] [review]
printers: set current page async
Comment 30 Felipe Borges 2016-02-17 14:38:28 UTC
Created attachment 321495 [details] [review]
printers: renew/cancel cups subscriptions asynchronously
Comment 31 Felipe Borges 2016-02-17 14:38:51 UTC
Created attachment 321496 [details] [review]
printers: get job attributes in a non-blocking manner (async)
Comment 32 Marek Kašík 2016-02-19 13:43:19 UTC
Review of attachment 321492 [details] [review]:

Thank you for the modifications. Please push it to master.
Comment 33 Marek Kašík 2016-02-19 14:35:54 UTC
Review of attachment 321493 [details] [review]:

I have few more comments on this :)

::: panels/printers/cc-printers-panel.c
@@ +94,3 @@
   gint             subscription_id;
   guint            subscription_renewal_id;
+  gboolean         cups_connected;

You can use the cups_status_check_id as an indicator instead. Just run g_source_remove() on it if the connection succeed and set it to 0 so the timeout is removed.

@@ +3088,1 @@
         g_timeout_add_seconds (CUPS_STATUS_CHECK_INTERVAL, cups_status_check, self);

This should be really called only when the server is not available initially so it regularly checks whether it became online. Add a connection test here and run the timeout only when it fails. (so you should have 2 connection tests in this patch then, one before the initiation of the timeout and one in the body of the timeout)

::: panels/printers/pp-cups.c
@@ +99,3 @@
+  http_t *http;
+
+  http = httpConnectEncrypt (cupsServer (), ippPort (), cupsEncryption ());

You should close the connection once you successfully establish it. (btw, httpClose() checks the input for NULL so you don't need to place it into an if())

@@ +103,3 @@
+    g_task_return_boolean (task, TRUE);
+  else
+    g_task_return_boolean (task, FALSE);

It would be better to just call "g_task_return_boolean (task, http != NULL)" so there is less code :).
Comment 34 Marek Kašík 2016-02-19 15:43:20 UTC
Review of attachment 321494 [details] [review]:

::: panels/printers/cc-printers-panel.c
@@ +1136,3 @@
         gtk_builder_get_object (priv->builder, "notebook");
 
+      pp_cups_connection_test_async (cups, set_current_page, widget);

You need to ref the cups here. It is missing it when I run g-c-c without running CUPS (you need to place some defective values into /etc/cups/cupsd.conf so it is not able to start these days :) ).

@@ +1227,3 @@
     }
 
+  g_object_unref (cups);

Could you move this closer to a code where it is used?
Comment 35 Felipe Borges 2016-02-23 15:22:48 UTC
Created attachment 321986 [details] [review]
printers: do async connection test during launch of panel

Thank you for your review, Marek.

(In reply to Marek Kašík from comment #33)
> Review of attachment 321493 [details] [review] [review]:
> 
> ::: panels/printers/cc-printers-panel.c
> @@ +3088,1 @@
>          g_timeout_add_seconds (CUPS_STATUS_CHECK_INTERVAL,
> cups_status_check, self);
> 
> This should be really called only when the server is not available initially
> so it regularly checks whether it became online. Add a connection test here
> and run the timeout only when it fails. (so you should have 2 connection
> tests in this patch then, one before the initiation of the timeout and one
> in the body of the timeout)

My idea while keeping this behavior was that the timeout would get removed as soon as the connection test succeed.

Anyhow, in the last version of the patch I have added a second connection test as you suggested.
Comment 36 Felipe Borges 2016-02-24 16:35:27 UTC
Created attachment 322257 [details] [review]
printers: set current page async
Comment 37 Felipe Borges 2016-02-24 16:35:51 UTC
Created attachment 322258 [details] [review]
printers: renew/cancel cups subscriptions asynchronously
Comment 38 Felipe Borges 2016-02-24 16:36:35 UTC
Created attachment 322259 [details] [review]
printers: get job attributes in a non-blocking manner (async)

Updated all the patches to match the changes made on the previous ones.
Comment 39 Marek Kašík 2016-02-25 11:21:20 UTC
Review of attachment 321986 [details] [review]:

(In reply to Felipe Borges from comment #35)
> Created attachment 321986 [details] [review] [review]
> printers: do async connection test during launch of panel
> 
> Thank you for your review, Marek.
> 
> (In reply to Marek Kašík from comment #33)
> > Review of attachment 321493 [details] [review] [review] [review]:
> > 
> > ::: panels/printers/cc-printers-panel.c
> > @@ +3088,1 @@
> >          g_timeout_add_seconds (CUPS_STATUS_CHECK_INTERVAL,
> > cups_status_check, self);
> > 
> > This should be really called only when the server is not available initially
> > so it regularly checks whether it became online. Add a connection test here
> > and run the timeout only when it fails. (so you should have 2 connection
> > tests in this patch then, one before the initiation of the timeout and one
> > in the body of the timeout)
> 
> My idea while keeping this behavior was that the timeout would get removed
> as soon as the connection test succeed.

In that case you would run this twice:
  actualize_printers_list (self);
  attach_to_cups_notifier (self);


> Anyhow, in the last version of the patch I have added a second connection
> test as you suggested.

Thanks.

Except those few comments it looks good.

::: panels/printers/cc-printers-panel.c
@@ +2789,3 @@
+  PpCups                 *cups = PP_CUPS (source_object);
+
+  priv = self->priv = PRINTERS_PANEL_PRIVATE (self);

Change this to "priv = self->priv;" please, I don't know what I was thinking about then.

@@ +2813,1 @@
   priv = self->priv = PRINTERS_PANEL_PRIVATE (self);

Ditto.

@@ +2816,3 @@
+  pp_cups_connection_test_async (cups, cups_status_check_cb, self);
+
+  return priv->cups_status_check_id == 0;

This is why it doesn't work for me. You have to have "!= 0" here. You can test it by setting a nonsense value in cupsd.conf and restarting cups so it fail and is not available. Once you restore it and restart it. You should see the printers in about 5 seconds.

@@ +2827,3 @@
+  CcPrintersPanel       *self = (CcPrintersPanel*) user_data;
+  gboolean               success;
+  PpCups                *cups = PP_CUPS (source_object);

Indent these 3 declarations by one more space.
Comment 40 Marek Kašík 2016-02-25 11:40:45 UTC
Review of attachment 322257 [details] [review]:

This looks good. Lets get it into 3.22 so wait until 3.20 branch is created and push it to master then (after the previous patch will be pushed there). Thanks
Comment 41 Felipe Borges 2016-02-25 13:19:01 UTC
Created attachment 322369 [details] [review]
printers: do async connection test during launch of panel

Thanks for the review.

(In reply to Marek Kašík from comment #39)
>
> @@ +2816,3 @@
> +  pp_cups_connection_test_async (cups, cups_status_check_cb, self);
> +
> +  return priv->cups_status_check_id == 0;
> 
> This is why it doesn't work for me. You have to have "!= 0" here. You can
> test it by setting a nonsense value in cupsd.conf and restarting cups so it
> fail and is not available. Once you restore it and restart it. You should
> see the printers in about 5 seconds.

Good tip! Worked for me :)
Comment 42 Marek Kašík 2016-02-25 13:56:28 UTC
Review of attachment 322369 [details] [review]:

Thank you for the changes. Push it to master once the 3.20 branch is created.
Comment 43 Bastien Nocera 2016-02-25 14:01:23 UTC
Why wait for the 3.22?
Comment 44 Marek Kašík 2016-02-25 14:07:35 UTC
(In reply to Bastien Nocera from comment #43)
> Why wait for the 3.22?

I wasn't sure whether this is more bug fix or a feature so I wanted to stay on safe side. But if it is ok with you, we can get it into 3.20.
Comment 45 Bastien Nocera 2016-02-25 14:09:15 UTC
We'll have enough time to fix it if it causes problems, and it's definitely a bug fix to guard against broken networks and servers.
Comment 46 Marek Kašík 2016-02-25 14:12:35 UTC
Ok then. Felipe, push the patches into the master please. Thanks.
Comment 47 Marek Kašík 2016-02-25 14:50:09 UTC
Review of attachment 322258 [details] [review]:

The problem here is that the functions handling subscriptions are not async. Lets create pp_cups_cancel_subscription_async|finish() and pp_cups_renew_subscription_async|finish() pairs based on threaded GTask. You won't need the connection tests in that case, you'll just check whether the requests were successful.
Comment 48 Felipe Borges 2016-02-25 15:18:42 UTC
Attachment 322257 [details] pushed as 1d36554 - printers: set current page async
Attachment 322369 [details] pushed as 0a7cfa4 - printers: do async connection test during launch of panel
Comment 49 Marek Kašík 2016-02-26 11:38:31 UTC
Review of attachment 322259 [details] [review]:

::: panels/printers/cc-printers-panel.c
@@ +300,3 @@
+              job_printer_uri = g_variant_get_string (g_variant_get_child_value (printer_uri, 0), NULL);
+
+              if (job_originating_user_name && job_printer_uri &&

Check these against NULL please.

@@ +434,3 @@
           httpClose (http);
         }
       g_free (job_uri);

I think that the code below this should replace the code above this.

@@ +1162,3 @@
   cups_job_t             *jobs = NULL;
   GtkWidget              *widget;
+  PpCupsDests            *cups_dests;

This is not needed I guess.

::: panels/printers/pp-job.c
@@ +299,3 @@
+            {
+              const GVariantType *type = NULL;
+              GVariant           *values[n_attrs];

Maybe I'm too old but this doesn't look like something from C. Change it please.

@@ +322,3 @@
+
+                    for (i = 0; i < n_attrs; i++)
+                      values[i] = g_variant_new_string ((ippGetString (attr, i, NULL)));

You can save some brackets here.

@@ +345,3 @@
+
+                  default:
+                    /* do nothing (switch w/ enumeration type) */

How is the nothing propagated to the g_variant_builder_add() ?

@@ +360,3 @@
+  g_free (job_uri);
+
+  g_task_return_pointer (task, attributes, g_object_unref);

GVariant is not an object.
Comment 50 Felipe Borges 2016-03-01 11:01:21 UTC
Created attachment 322737 [details] [review]
printers: cancel cups subscriptions asynchronously

I have split the renew/cancel patches into two different ones.

The renew subscription asynchronously patch comes next.
Comment 51 Felipe Borges 2016-03-01 12:38:23 UTC
Created attachment 322751 [details] [review]
printers: renew cups subscriptions asynchronously
Comment 52 Marek Kašík 2016-03-02 16:04:20 UTC
Review of attachment 322737 [details] [review]:

Thank you for the patch. I have several comments here which are just cosmetic issues. Once you tackle them feel free to commit the patch.

::: panels/printers/cc-printers-panel.c
@@ +460,3 @@
 
+ static void
+detach_from_cups_notifier_cb (GObject      *source_object,

Could you name this like subscription_cancel_cb() or similar so that it is related to the subscription cancellation?

::: panels/printers/pp-cups.c
@@ +150,3 @@
+    }
+
+  g_task_return_boolean (task, response != NULL && ippGetStatusCode (response) <= IPP_OK_BUT_CANCEL_SUBSCRIPTION);

I think that checking against IPP_OK is enough (see https://www.ietf.org/proceedings/50/I-D/ipp-indp-method-04.txt for the successful-ok-but-cancel-subscription - I'm not really sure what it is for).
Btw, here is quite comprehensive list of IPP-related stuff: http://www.iana.org/assignments/ipp-registrations/ipp-registrations.xhtml.

@@ +153,3 @@
+
+  if (response != NULL)
+    ippDelete (response);

ippDelete() checks the pointer for NULL itself.
Comment 53 Felipe Borges 2016-03-02 16:24:05 UTC
Comment on attachment 322737 [details] [review]
printers: cancel cups subscriptions asynchronously

Attachment 322737 [details] pushed as 9f9c63f - printers: cancel cups subscriptions asynchronously

Thanks!
Comment 54 Felipe Borges 2016-03-03 11:59:38 UTC
Created attachment 322967 [details] [review]
printers: get printer job attributes async
Comment 55 Marek Kašík 2016-03-03 12:29:23 UTC
Review of attachment 322751 [details] [review]:

Hi, this needs some work yet.

::: panels/printers/cc-printers-panel.c
@@ +420,2 @@
   if (priv->subscription_id > 0)
+    return G_SOURCE_REMOVE;

This part breaks the functionality. Lets make it work the way that we try initially to renew (create actually) the subscription and if it succeed then add the timeout which will regularly renew the subscription. If it won't succeed initially then it will be picked up later by the attach_to_cups_notifier() triggered by the timeout created in connection_test_cb().
The timeout for renewing of the subscription shouldn't be ever removed except when finishing the panel.

::: panels/printers/pp-cups.c
@@ +127,3 @@
 }
+
+/* Returns id of renewed subscription or new id */

This should be above the finish function.

@@ +167,3 @@
+      if (response != NULL && ippGetStatusCode (response) <= IPP_OK_CONFLICT)
+        {
+          if ((attr = ippFindAttribute (response, "notify-lease-duration",

Put branches of this if into brackets.

@@ +201,3 @@
+              result = ippGetInteger (attr, 0);
+          }
+      }

This block has wrong indentation.

@@ +204,3 @@
+
+    if (response)
+      ippDelete (response);

You don't need to check for NULL here.

@@ +223,3 @@
+  subscription_data = g_slice_new (CRSData);
+  subscription_data->id = subscription_id;
+  subscription_data->events = events;

You have to copy the list, use g_strdupv() once you end the passed list with NULL.
Don't forget to free it in the crs_data_free().

::: panels/printers/pp-cups.h
@@ +75,3 @@
+void         pp_cups_renew_subscription_async  (PpCups              *cups,
+                                                gint                 subscription_id,
+                                                const char * const  *events,

Lets make the events list a list ended by NULL so you don't need to pass its length.
Comment 56 Marek Kašík 2016-03-03 13:32:30 UTC
Review of attachment 322967 [details] [review]:

It just needs some finishing touches.

::: panels/printers/pp-job.c
@@ +303,3 @@
+              gint                 range_uppervalue;
+
+              values = g_malloc (n_attrs * sizeof (GVariant*));

g_new() would be more suitable.

@@ +351,3 @@
+                }
+
+              g_variant_builder_add (&builder, "{sv}",

Call this only if type != NULL. Otherwise you are passing bunch of uninitialized pointers in "values". And put the call into brackets since the if branch will take more than 1 line.

@@ +354,3 @@
+                                     attributes_names[j],
+                                     g_variant_new_array (type, values, n_attrs));
+

g_free() the "values" here so you don't leak memory.
Comment 57 Felipe Borges 2016-03-03 14:47:54 UTC
Created attachment 322985 [details] [review]
printers: get printer job attributes async
Comment 58 Marek Kašík 2016-03-03 14:51:46 UTC
Review of attachment 322985 [details] [review]:

Thank you for the changes. Push it to master please.
Comment 59 Felipe Borges 2016-03-03 15:08:48 UTC
Comment on attachment 322985 [details] [review]
printers: get printer job attributes async

Attachment 322985 [details] pushed as dc7b78c - printers: get printer job attributes async

Thanks!
Comment 60 Felipe Borges 2016-03-03 16:51:46 UTC
Created attachment 323009 [details] [review]
printers: renew cups subscriptions asynchronously
Comment 61 Felipe Borges 2016-03-04 09:32:00 UTC
Created attachment 323072 [details] [review]
printers: renew cups subscriptions asynchronously

*we should always renew the cups subscription.
Comment 62 Felipe Borges 2016-03-04 09:46:24 UTC
Created attachment 323073 [details] [review]
printers: renew cups subscriptions asynchronously

*It should create the subscription in the first place.
Comment 63 Felipe Borges 2016-03-04 10:41:45 UTC
Created attachment 323076 [details] [review]
printers: renew cups subscriptions asynchronously

*we were not properly subscribing to the desirable events.
Comment 64 Felipe Borges 2016-03-07 11:31:39 UTC
Created attachment 323255 [details] [review]
printers: renew cups subscriptions asynchronously
Comment 65 Marek Kašík 2016-03-08 14:57:40 UTC
Review of attachment 323255 [details] [review]:

Thank you for the changes. It works good here except one very special case I forgot to test before. It is when the operation in the new thread takes more time than usually. It can happen that the panel is disposed before the thread returns and the callback will try to access structures which are already freed. It is easy to reproduce this by placing e.g. "g_usleep (5000000)" just before the g_task_return_*() function and go to the overview right after you open Printers panel. We need a GCancellable for this added in the pp_cups_renew_subscription_async(). You have to check then what is returned by g_task_return_int() when the operation is cancelled and continue accordingly (e.g. don't set priv if the operation was already cancelled).

::: panels/printers/cc-printers-panel.c
@@ +445,1 @@
+  return TRUE;

Use G_SOURCE_CONTINUE here since the return value is used by the timeout only.

::: panels/printers/pp-cups.c
@@ +202,3 @@
+  ipp_t           *request;
+  ipp_t           *response = NULL;
+  gint            result = -1;

Indentation.

@@ +204,3 @@
+  gint            result = -1;
+
+  if (subscription_data->id >= 0)

I've checked the notify-subscription-id in RFC3995 and it states that the subscription id starts at 1 so you can safely use "subscription_data->id > 0" here.
Comment 66 Felipe Borges 2016-03-09 14:23:50 UTC
Created attachment 323515 [details] [review]
printers: renew cups subscriptions asynchronously
Comment 67 Marek Kašík 2016-03-09 17:40:43 UTC
Review of attachment 323515 [details] [review]:

Thank you for the modifications. The patch looks and works good now. Please push it to master.
Consider yet whether to remove the redundant lines. If it is your coding style then keep them.

::: panels/printers/cc-printers-panel.c
@@ +425,3 @@
+  gint                    subscription_id;
+
+

Redundant empty line.

@@ +471,1 @@
 

Ditto.
Comment 68 Felipe Borges 2016-03-10 10:10:22 UTC
Attachment 323515 [details] pushed as 16d32c4 - printers: renew cups subscriptions asynchronously

Thanks for your reviews!