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 720058 - Cancellation fails with push/pull when progress callbacks are used
Cancellation fails with push/pull when progress callbacks are used
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: client module
git master
Other Linux
: Normal normal
: ---
Assigned To: gvfs-maint
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2013-12-08 10:23 UTC by Ross Lagerwall
Modified: 2014-11-18 06:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
client: Use the correct serial with push/pull cancellation (4.76 KB, patch)
2013-12-08 10:34 UTC, Ross Lagerwall
reviewed Details | Review
client: Use the correct serial with file transfer cancellation (5.50 KB, patch)
2014-07-05 07:54 UTC, Ross Lagerwall
none Details | Review
client: Use the correct serial with file transfer cancellation (5.46 KB, patch)
2014-09-16 19:07 UTC, Ross Lagerwall
none Details | Review

Description Ross Lagerwall 2013-12-08 10:23:19 UTC
Cancelling works from the client point-of-view but the message doesn't get through to the daemon correctly.
Comment 1 Ross Lagerwall 2013-12-08 10:34:12 UTC
Created attachment 263737 [details] [review]
client: Use the correct serial with push/pull cancellation

When using Push and Pull with progress callbacks, the progress callbacks
increment the connection serial, so retrieve the serial immediately
after the Push/Pull method call so that the correct serial is used for
cancellation.
Comment 2 Ondrej Holy 2014-01-23 13:47:05 UTC
Review of attachment 263737 [details] [review]:

Thanks for the patch. It looks good to me, however don't know the serials deeply...

alex, could you look at please?
Comment 3 Alexander Larsson 2014-01-23 16:04:29 UTC
Review of attachment 263737 [details] [review]:

::: client/gdaemonfile.c
@@ +2930,3 @@
                                  copy_cb,
                                  &data);
+      serial = g_dbus_connection_get_last_serial (connection);

Don't you need this for the _copy and _move calls also?
Comment 4 Ross Lagerwall 2014-07-05 07:54:47 UTC
Created attachment 279941 [details] [review]
client: Use the correct serial with file transfer cancellation

When using Copy, Move, Push and Pull with progress callbacks, the
progress callbacks increment the connection serial, so retrieve the
serial immediately after the dbus method call so that the correct serial
is used for cancellation.
Comment 5 Ross Lagerwall 2014-07-05 07:55:49 UTC
(In reply to comment #3)
> Review of attachment 263737 [details] [review]:
> 
> ::: client/gdaemonfile.c
> @@ +2930,3 @@
>                                   copy_cb,
>                                   &data);
> +      serial = g_dbus_connection_get_last_serial (connection);
> 
> Don't you need this for the _copy and _move calls also?

Yes indeed. Fixed in latest patch.

Thanks for the review.
Comment 6 Alexander Larsson 2014-09-16 07:57:39 UTC
Review of attachment 279941 [details] [review]:

::: client/gdaemonfile.c
@@ +2978,3 @@
+                                                     serial);
+          else
+            _g_dbus_send_cancelled_sync (g_dbus_proxy_get_connection (G_DBUS_PROXY (proxy)));

The only way this would be reached with serial != 0 is when g_dbus_interface_skeleton_export() fails, and at that point we didn't send an operation to cancel. So, maybe we should just check for serial != 0 instead of "if (proxy &&" and not send a dbus cancel if serial is 0 at this point.
Comment 7 Ross Lagerwall 2014-09-16 19:07:53 UTC
Created attachment 286321 [details] [review]
client: Use the correct serial with file transfer cancellation

When using Copy, Move, Push and Pull with progress callbacks, the
progress callbacks increment the connection serial, so retrieve the
serial immediately after the dbus method call so that the correct serial
is used for cancellation.
Comment 8 Ross Lagerwall 2014-09-16 19:08:44 UTC
(In reply to comment #6)
> Review of attachment 279941 [details] [review]:
> 
> ::: client/gdaemonfile.c
> @@ +2978,3 @@
> +                                                     serial);
> +          else
> +            _g_dbus_send_cancelled_sync (g_dbus_proxy_get_connection
> (G_DBUS_PROXY (proxy)));
> 
> The only way this would be reached with serial != 0 is when
> g_dbus_interface_skeleton_export() fails, and at that point we didn't send an
> operation to cancel. So, maybe we should just check for serial != 0 instead of
> "if (proxy &&" and not send a dbus cancel if serial is 0 at this point.

Thanks for the review, I have updated the patch accordingly.
Comment 9 Philip Langdale 2014-11-16 17:14:17 UTC
Can this get committed? It's still very much an issue.
Comment 10 Ross Lagerwall 2014-11-18 06:31:41 UTC
(In reply to comment #9)
> Can this get committed? It's still very much an issue.

The last patch I uploaded accidentally did not include all the changes, but I pushed a version with Alex's suggestions to master (a336ef412ace2f9fc169185c48bca657e28c646e) and stable (4b85af0d4a7ad69773839bd248284beb80e4bcac).  Let me know if it's fixed.