Bug 72319 - test-file-transfer-channel fails with glib 2.39
Summary: test-file-transfer-channel fails with glib 2.39
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/xc...
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-12-04 17:02 UTC by Sebastien Bacher
Modified: 2014-03-13 13:54 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
patch (2.18 KB, patch)
2013-12-04 17:07 UTC, Allison Lortie (desrt)
Details | Splinter Review

Description Sebastien Bacher 2013-12-04 17:02:05 UTC
The test is failing under glib 2.39

"(/tmp/build/telepathy-glib/telepathy-glib-0.22.0/tests/dbus/.libs/lt-test-file-transfer-channel:21920): tp-glib/channel-DEBUG: tp_file_transfer_channel_state_changed_cb: File transfer state changed: old state = 1, state = 3, reason = 1, requested = yes, in_stream = present, out_stream = not present
(/tmp/build/telepathy-glib/telepathy-glib-0.22.0/tests/dbus/.libs/lt-test-file-transfer-channel:21920): tp-glib/channel-DEBUG: start_transfer: Client socket connected immediately
(/tmp/build/telepathy-glib/telepathy-glib-0.22.0/tests/dbus/.libs/lt-test-file-transfer-channel:21920): tp-glib/channel-DEBUG: client_socket_connected: File transfer socket connected
** (/tmp/build/telepathy-glib/telepathy-glib-0.22.0/tests/dbus/.libs/lt-test-file-transfer-channel:21920): DEBUG: state_notify_cb: state_notify_cb was triggered
(/tmp/build/telepathy-glib/telepathy-glib-0.22.0/tests/dbus/.libs/lt-test-file-transfer-channel:21920): tp-glib/channel-DEBUG: splice_stream_ready_cb: splice operation failed: Error sending data: Broken pipe
** (/tmp/build/telepathy-glib/telepathy-glib-0.22.0/tests/dbus/.libs/lt-test-file-transfer-channel:21920): DEBUG: file_provide_cb: file_provide_cb reached

(/tmp/build/telepathy-glib/telepathy-glib-0.22.0/tests/dbus/.libs/lt-test-file-transfer-channel:21920): GLib-CRITICAL **: g_main_loop_quit: assertion 'loop != NULL' failed"


That's happening since https://git.gnome.org/browse/glib/commit/?id=4e9e7d0cba53a711bd650e9a5e28452b93f0d849 but seems to be a bug on the tp-glib side
Comment 1 Allison Lortie (desrt) 2013-12-04 17:07:19 UTC
Created attachment 90258 [details] [review]
patch
Comment 2 Xavier Claessens 2013-12-04 17:30:46 UTC
There is an ambiguity of when _accept_file_async() and _provide_file_async() are supposed to complete. The unit tests assert that in completes when the state moved to TP_FILE_TRANSFER_STATE_PENDING which is before the actual streaming of the file happens, and that's what actually happens on the success code path.

However on the failure code path, TpFileTransferChannel assume the operation is still ongoing until the splice operation ends so that would mean that _accept/provide_file_async() covers the whole file transfer process and returns when the state moved to TP_FILE_TRANSFER_STATE_COMPLETED.
Comment 3 Xavier Claessens 2013-12-04 19:29:25 UTC
Made a more complete fix: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=fix-ft
Comment 4 Guillaume Desmottes 2013-12-27 11:34:09 UTC
(In reply to comment #3)
> Made a more complete fix:
> http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=fix-ft

You mentioned twice "tp_file_transfer_channel_accept_file_async" in the commit message.

Ignoring streaming errors seem pretty nasty. :\
Comment 5 Xavier Claessens 2014-01-03 15:45:10 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > Made a more complete fix:
> > http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=fix-ft
> 
> You mentioned twice "tp_file_transfer_channel_accept_file_async" in the
> commit message.

Fixed, and fixed the "steaming errors" typo Simon saw as well.

> Ignoring streaming errors seem pretty nasty. :\

Better ignoring errors, than crashing on error as it always did.

I follow-up branch should add an "error" signal (both in C API, and in spec IMHO), and a cancel method.

OTOH, I think we should just give up on that over-engineered interface, and just give the file URI to the CM and let it do all the work. The current iface cannot even be implemented on haze...
Comment 6 Xavier Claessens 2014-01-03 15:46:01 UTC
(In reply to comment #5)
> I follow-up branch should add an "error" signal (both in C API, and in spec
> IMHO), and a cancel method.
> 
> OTOH, I think we should just give up on that over-engineered interface, and
> just give the file URI to the CM and let it do all the work. The current
> iface cannot even be implemented on haze...

Oh, and I'm not volunteering to do that work atm :P
Comment 7 Guillaume Desmottes 2014-01-06 09:58:58 UTC
(In reply to comment #5)
> Better ignoring errors, than crashing on error as it always did.

Fair enough. ++

> OTOH, I think we should just give up on that over-engineered interface, and
> just give the file URI to the CM and let it do all the work. The current
> iface cannot even be implemented on haze...

I've been advocating that since the first drafts of this API...
Comment 8 Xavier Claessens 2014-01-06 21:05:51 UTC
Branch merged. I opened bug #73334 and #73333 for the follow up work that can be done on TpFileTransferChannel, if someone cares.
Comment 9 Simon McVittie 2014-03-13 13:54:53 UTC
This was originally fixed in 0.23.1. I cherry-picked it for 0.22.2.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.