search

TelepathyIM / telepathy-qt

Telepathy Qt bindings
https://telepathy.freedesktop.org
GNU Lesser General Public License v2.1
25 stars 14 forks source link

Register object before the service gets exposed on DBus #20

Closed boiko closed 8 years ago

boiko commented 8 years ago

This is required for services to get properly registered when using QtDBus >= 5.6

Kaffeine commented 8 years ago

Merged. Thanks! f81ae2d33ff36baa7938b4f89fa51372c9ff1fcc

gkiagia commented 8 years ago

Why would it be required to register objects before the service? That sounds like it would actually break most telepathy objects, which are not associated with a service of their own and they can be created much after the original service names have been registered.

gkiagia commented 8 years ago

Also, a unit test would be useful...

Kaffeine commented 8 years ago

Why would it be required to register objects before the service?

Because applications, which use the service will try to access the service objects right on the service registration? And it will cause errors if there are no mandatory objects on the service?

That sounds like it would actually break most telepathy objects, which are not associated with a service of their own and they can be created much after the original service names have been registered.

Before and after the change, the method DBusService::registerObject() calls both registerService() and registerObject(). How it could work the way you said? The original order is return registerService() ? registerObject() : false;, the new order is return registerObject() ? registerService() : false;. The new order let a service to be exposed with well-known name in a complete form. I don't see why it is bad to register objects for unique (numerical) service name.

gkiagia commented 8 years ago

Because applications, which use the service will try to access the service objects right on the service registration? And it will cause errors if there are no mandatory objects on the service?

Ah I see, so it's a race condition, not an actual requirement of QtDBus. The first comment here sounded like QtDBus does not allow you to register objects after registering the service, hence why I said it would break other objects.

I don't see why it is bad to register objects for unique (numerical) service name.

It is not bad. You misunderstood my comment, like I misunderstood boiko's.

boiko commented 8 years ago

Hi

On Wed, Nov 23, 2016 at 6:24 AM, George Kiagiadakis < notifications@github.com> wrote:

Because applications, which use the service will try to access the service objects right on the service registration? And it will cause errors if there are no mandatory objects on the service?

Ah I see, so it's a race condition, not an actual requirement of QtDBus. The first comment here sounded like QtDBus does not allow you to register objects after registering the service, hence why I said it would break other objects.

Sorry, I could have made the commit message cleaner: QtDBus uses threads internally now, so once you call registerService() chances are that it is going to appear on the bus immediately. That's why registering the objects before the service is necessary (for objects that are expected to be available when the service is dbus-activated, etc)

I don't see why it is bad to register objects for unique (numerical) service name.

It is not bad. You misunderstood my comment, like I misunderstood boiko's.

Regarding the unit tests, I wanted to write one, but I don't see how to properly test this case, if you have a suggestion, please let me know.

Cheers

Boiko

Kaffeine commented 8 years ago

I think that there is no easy way to reliably test this race-condition.