In the case we are both an observer and an handler, it common in observe_channels() to claim them. In that case we should start handling from the claim_cb and we should NOT get handle_channels(). So typically code would do: tp_channel_dispatch_operation_claim_with_async (dispatch_operation, self, claim_cb, user_data); tp_observe_channels_context_accept (context); Which would be correct if tp_channel_dispatch_operation_claim_with_async() was calling Claim dbus call directly. But since it first do a tp_proxy_prepare_async() on the DO, the return from dbus call done in tp_observe_channels_context_accept() gets to MC first and it will then dispatch the channel to anyone able to.
Hm, and I cannot move the tp_observe_channels_context_accept() into claim_cb() because then MC waits for the dbus ObserveChannels dbus call to timeout before returning from Claim().
The only solution I see is to remove the preparation of DO in claim_with(), why would it not be ready anyway?
Here is a branch that remove useless preparation: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=do-race
Also added a patch to make TpObserverChannelsContext prepare its CDO.
For the record, smcv was concerned about this patch because it means that all observers will have to prepare the TpChannelDispatchOperation even if they don't need it if they are not going to handle it. That was a big issue before factory changes because the TpChannel objects that TpCDO creates were not shared with those prepared by the TpBaseClient. But now that they are the same objects, preparing TpCDO is just one GetAll call...
What happen if user calls tp_channel_dispatch_operation_claim_with_async() on an unprepared object? _tp_base_client_now_handling_channels (client, self->priv->channels); will fail and leave/destroy variant won't work? We should change documentation and add a g_return_if_fail() test checking if the CDO is actually prepared. Any reason why a user would want to use tp_channel_dispatch_operation_new()? Shouldn't we deprecate it? Did you double check that tp-glib is always preparing it itself?
(In reply to comment #6) > What happen if user calls tp_channel_dispatch_operation_claim_with_async() on > an unprepared object? _tp_base_client_now_handling_channels (client, > self->priv->channels); will fail and leave/destroy variant won't work? Actually in that case self->priv->channels is NULL, so claim_with will do a g_return_if_fail inside _tp_base_client_now_handling_channels, and leave/destroy will just crash when doing self->priv->channels->len. So you're right that those APIs should g_return_if_fail(tp_proxy_is_prepared()) to early-return. > Any reason why a user would want to use tp_channel_dispatch_operation_new()? > Shouldn't we deprecate it? Indeed, I don't see any reason for an app to use that. This is the same story than tp_channel_new() and all other TpProxy subclasses constructors. We actually even kept _tp_simple_client_factory_ensure_channel_dispatch_operation private because there were no reason it to be useful. I would just leave it for now, we should deprecate all proxy constructors at once, IMO. I'm not sure we have a bug for that, though. > Did you double check that tp-glib is always preparing it itself? The only place a CDO is created is from TpBaseClient, and I've fixed that case, yes.
Looks to me for master then. Not sure if we should push this in stable as well or not (risk to break app which was using tp_cdo_new() manually).
Ok, added the g_return_if_fail, and merged. I'm pretty sure this should be safe for stable branch, but indeed since it change behaviour of stable API, it could be considered dangerous.
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.