Connection.GetObject: do not check that the bus name exists #33
Conversation
This check did not exist in 0.7, but was added in e0eed78 before 0.8. When D-Bus service activation is used, the desired bus name is not going to exist at this point; when we call a method, the dbus-daemon transparently starts the service before delivering the message. This is particularly important for services that are not normally running before they are needed, such as polkit and systemd-hostnamed. Calling NameHasOwner also doubles the number of method calls if you're only making one method call on the object, and has a race condition: if the service exits between the successful NameHasOwner call and the next method call, the caller will still have to deal with that. Working around the check by doing a StartServiceByName first is inefficient, and also introduces a race condition: if the service exits between StartServiceByName success and NameHasOwner, then GetObject would still return null.
In the OO languages I'm most familiar with (C# is not one of them), removing a protected method is technically an ABI break: a third-party subclass could conceivably be calling it. (Removing a private method isn't usually an ABI break, though.) I don't know enough about the finer points of C# to know whether there is some reason why it would not be an ABI break to revert e0eed78 fully; if it wouldn't, or if it would but the dbus-sharp maintainers think it is better to do that anyway, I'm not going to argue against that. |
That protected method was introduced in the commit to revert. |
Yes, and that's why reverting the commit would be an ABI break. |
|
Right, but this PR would seem to be a fix on top of a fix, but if it's essentially a revert, with the only difference that you leave the method there, to not break API, then you're unfixing what e0eed78 was fixing. Can you propose a fix for your problem that can also fix the problem that e0eed78 was solving? |
|
I don't think e0eed78 made things better; what it seems to be aiming for can never be fully reliable, and I think the 0.7 API was a better way to interact with D-Bus services. What you propose in #40 mitigates the problems with the 0.8 version, but I still think the 0.7 API was better. The equivalent of GetObject() in most other D-Bus bindings (GDBus, QtDBus, dbus-python, dbus-glib, etc.), and GetObject() in dbus-sharp 0.7, always returns a valid proxy object. If the service isn't running and can't be activated, method calls on that proxy object will get an error reply (exception); but D-Bus method calls can always fail anyway (for instance because they time out, or the service crashes, or the service has changed its D-Bus interfaces and is now incompatible with the client), so clients without graceful exception handling are not correctly-implemented. The implementation in 0.8 inserts an extra method call (round-trip) into each GetObject(), in order that it can return null if dbus-sharp thinks method calls won't work. This means that in addition to checking errors from the actual method calls, callers also need to check for a null return from GetObject(); and that if dbus-sharp thinks a method call won't work when actually it will, there is no opportunity to try it (the bug that prompted this PR and #40). The null return doesn't actually seem to provide any benefit to callers: callers still have to check for errors on method calls after a non-null return, because a non-activatable service might exit after the call to NameHasOwner returns true but before the subsequent method call (or the call might time out, etc.). Your implementation in #40 adds either 1 or 2 extra method calls (round-trips) into each GetObject(), in order that it can return null. It's an improvement over 0.8, because it would make on-demand services like polkit and systemd-hostnamed work, but callers still don't actually get any benefit from it: method calls still need error-checking, because a non-activatable service might exit after NameHasOwner returns true, or an activatable service's activation might fail. Minimizing blocking on round-trips is a very common piece of D-Bus optimization advice, so it seems undesirable to have additional "hidden" blocking round-trips for little or no practical benefit. |
|
Fair enough. I feel that you could add some bits of your latest argumentation, to the commit message. Plus, marking CheckBusNameExists() as [Obsolete ("Not used anymore, plus base implementation does nothing.")] or something like that. |
This check did not exist in 0.7, but was added in e0eed78 before 0.8.
When D-Bus service activation is used, the desired bus name is not
going to exist at this point; when we call a method, the dbus-daemon
transparently starts the service before delivering the message.
This is particularly important for services that are not normally
running before they are needed, such as polkit and systemd-hostnamed.
Calling NameHasOwner also doubles the number of method calls if you're
only making one method call on the object, and has a race condition:
if the service exits between the successful NameHasOwner call and the
next method call, the caller will still have to deal with that.
Working around the check by doing a StartServiceByName first is
inefficient, and also introduces a race condition: if the service
exits between StartServiceByName success and NameHasOwner, then
GetObject would still return null.