optionnal signal registration during proxy lifetime

[antoine-girard-vallee]

It would be really useful in our usecases to be able to register/unregister signals freely during the lifetime of a proxy.

The uponSignal() helper methods seem to be intended for use in bulk right during init before calling finishRegistration() (at least by looking at generated stubs).

I propose allowing direct registration using connection_->registerSignalHandler() in Proxy::registerSignalHandler if the initial registration is complete. Paired with muteSignal(), it allows handling available signals only when needed.

I have a simple working patch locally tested on top of 1.2.0 that I could send up for review.
If this feature sounds helpful, I would be interested in discussing something that would integrate neatly in the stub generation, like an annotation for signals. With my patch, signals have to be handled separately as the stub generator registers all signals automatically.

Thank you for your time, and sorry if I missed something obvious in sdbus-cpp.

Antoine Girard-Vallée

[sangelovic]

Hi @antoine-girard-vallee, thanks for your patience with my reply :) and for sharing your proposal.

It's true that underlying sd-bus library enables us to register/unregister signal handlers at any time during run time (which is not so simple for e.g. method call handlers on server side; it may be their intention). So we could really physically register signal handlers right away upon calling the registration function. We don't truly need finishRegistration() here. The function was introduced to consistently mirror the IObject::finishRegistration(), and for the need of generated stub code, where we need to decouple registration of handlers from its activation.

The former we could solve by removing finishRegistration() also in IObject API... this would actually simplify IObject interface. But that needs more investigation as it's not so simple. It's another story.

The latter is more important, because when generated proxy class is registering virtual functions as signal handlers in its constructor, the functions are pure at that moment. They are implemented in the derived class, which then does the finishRegistration(), to close the tiny buggy gap for potential pure virtual function call. However, we can solve this in a different way. The registration of signals will be placed in a dedicated method of generated proxy class. Instead of registerProxy(), the user will call that method from his constructor to do the registration. Or he can do the registration of signals himself, at a finer granularity (for example, he can register to signal A, but leave signal B out for the moment, and register to it upon an event later on).

What do you think of this? Would this meet your needs? I can prepare a PR for you.

[antoine-girard-vallee]

Thank you for response!

My toughts:

finishRegistration(), to close the tiny buggy gap for potential pure virtual function call.

I see. While reading proxy.cpp I wondered why finishRegistration() was necessary, now I understand.

The registration of signals will be placed in a dedicated method of generated proxy class.
Instead of registerProxy(), the user will call that method from his constructor to do the registration.

If I get this correctly, on top of calling registerProxy(), the implementer of the stub MAY call registerSignals()
to do the proxy_.uponSignal calls that were previously done in the constructor of the stub. OR he may pick his own signals when he pleases.

1: How will it keep backwards compatibility? Does this mean that all proxy stub implems have to do this call to get their signals?
If it is optional, depending on how its implemented the automatic registration of signal may silently stop being done for all
implementations without changes.
2: I feel having the implementer not forget do do a call to get the thing working adds friction. For example, forgetting registerProxy()
is an issue I've seen.

he can register to signal A, but leave signal B out for the moment, and register to it upon an event later on).

To allow registering later on, something similar to my patch will need to be done in Proxy.cpp to allow dbus matches to be added
after finishRegistration().

I believe that keeping current behavior, but adding an optional flag to the xml to prevent automatic registration of signals would be the simplest and be transparent for current implementations. This, combined with my patch to registerSignal() would make the whole thing work.

The optional signals would then only do the pure virtual stub of the signal handler. In our case, we set/unset callback handlers for our optional signals, and it would be great if the stub generator could do this boilerplate automatically. I could do such a patch later on in november as I don't have the time right now. However, this would make optional signal handling pretty opinionated.

I can prepare a PR for you.

That would be amazing thank you! I was expecting to have to work more for this feature :P
I can still send my work in if it can save you a bit of time. My naive patch assumes finishRegistration() and later optionnal calls to uponSignal are sequential, but since its done during construction I think its an OK limitation.

Thank you again for your time,

Antoine Girard-Vallée

[sangelovic]

@antoine-girard-vallee

If I get this correctly, on top of calling registerProxy(), the implementer of the stub MAY call registerSignals()

Not exactly. Implementers of classes inheriting from stubs would still in their constructor call registerProxy() of the ProxyInterfaces class like they do now. But internally, this method would call new registerProxy() methods of individual stubs, where they would do the uponSignal registration (as opposed to signal registrations done at a fixed time in the fixed place -- the stub constructor). Alternatively, instead of calling this method, they can register individual signals.

Does this mean that all proxy stub implems have to do this call to get their signals?

Yes, but just like now. Today they also have to call registerProxy(). This is the limitation of the design based on inheritance from stubs.

If it is optional, depending on how its implemented the automatic registration of signal may silently stop being done for all
implementations without changes.

Today is the same situation. They have to call registerProxy() for signals to work. Nothing will change here.

I feel having the implementer not forget do do a call to get the thing working adds friction.

Exactly, that's why doing the registration truly directly within registerSignalHandler() makes the situation better here, because finishRegistration() will fall off. So it will not be needed anymore on basic and convenience API level. It will only remain on IObject side. But there I'd also then like to re-design to remove finishRegistration(). So this would completely solve this and the issue of forgetting to call finishRegistration() you've seen (I've seen, too, e.g. #149).

How will it keep backwards compatibility?

That's a good point. Backwards compatibility is possible to be kept for basic and convenience API level. But for generated stubs, clients would need to re-generate the stubs with this approach. That's the downside of this. Hence this would need to be released as a new major release. I anyway have a list of ideas to implement/refactor that would probably break small things here and there. Removing IObject::finishRegistration could be part of that...

To sum up, I think this would be the correct path to go. It would make the API simpler, and more straight-forward. Registration function would really do the full registration. There would not be additional complexity of an implicit proxy states like "before registration", "finished registration", which makes it even simpler. No special, sdbus-c++ behavior-specific extensions to the signal part of the IDL XML file. I agree with you that your approach, if I understand it correctly, would be simple to implement, too, and backwards-compatible, but I think it would add special behaviors (don't want to say hacks explicitly :) ) bent to be built on top current limited API of 2-stage registration. Do you have some additional thoughts?

Btw., you can even with current v1.2 API achieve what you need. You can freely unregister signals that were already registered, for that we already have the API. When you need to re-register some signals again, you can call registerSignalHandler again and then call finishRegistration() again. It should work :) (although it was not designed with that in mind)

[antoine-girard-vallee]

this method would call new registerProxy() methods of individual stubs, where they would do the uponSignal registration

Ok I get it now and I agree with you, it makes more sense. UponSignal does the register directly, but pure virtual problem is avoided this way.

don't want to say hacks explicitly :)

I agree my solution is kind of hackish :), but I optimized for minimal impact, which doesn't seem to be that much of an issue.
We do use a lot of sdbus-cpp in our systems but we regenerate stubs per build so its not a problem for us either.

Btw., you can even with current v1.2 API achieve what you need.

from what I saw it would call sd_bus_add_match again for the whole list.
signalData->slot = connection.registerSignalHandler( destination_
at this line, I believe it would call another sd_bus_add_match for the same rule before deleting the slot and calling the remove match callback. I didn't want to try messing with that (I don't know libdbus that much) and anyway, as an embedded guy, re-iterating through all signals made me frown ;).

It would make the API simpler, and more straight-forward.

Agreed, that sounds great. I will keep my patch in our buildsystem until this is in master.

A bit off-topic, but my proposition for optional signal boilerplate (for signal callback register/unregister) in stubs with some xml annotation is totally compatible with your intended fix. If I was to implement it at some point in November, would you be open to look at an MR for it? We will use it locally and I might as well upstream.

[sangelovic]

I believe it would call another sd_bus_add_match for the same rule before deleting the slot

Yes, it would first register the new callback handler and then immediately release the old registration. And proceed with the next one in the loop... I think it's not so bad :) The point is it should be usable, should you need this functionality already now.

... my proposition for optional signal boilerplate (for signal callback register/unregister) ... Would you be open to look at an MR for it?

Yes, I'm at the moment not entirely sure what you mean by that, you can show me an example or we can just wait till November for your PR and then see and discuss.

[antoine-girard-vallee]

Yes, I'm at the moment not entirely sure what you mean by that, you can show me an example or we can just wait till November for your PR and then see and discuss.

Ok I will send you this once I get some free time for this. Thank you.