Connection.GetObject: do not check that the bus name exists

[smcv]

This check did not exist in 0.7, but was added in e0eed784d7 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.

[knocte]

@smcv , if you're removing that if clause, why not revert https://github.com/mono/dbus-sharp/commit/e0eed784d7de8ec93e24da66ada20d1a6c45673c altogether? (Because, it seems the CheckBusNameExists method is only used here.)

[smcv]

why not revert e0eed78 altogether? (Because, it seems the CheckBusNameExists method is only used here.)

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.

[knocte]

removing a protected method is technically an ABI break

That protected method was introduced in the commit to revert.

[smcv]

That protected method was introduced in the commit to revert.

Yes, and that's why reverting the commit would be an ABI break.

[knocte]

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?

[smcv]

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.

[knocte]

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.