-
-
Notifications
You must be signed in to change notification settings - Fork 131
Streams for D-Bus signals #1828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@sdroege This PR doesn't actually do what it's said to do yet 😬 but I'd like to have some early feedback on the current commit which replaces the This change is a bit of a prerequisite for actually receiving a signal as stream, because making a signal subscription droppable makes it a lot easier to manage the subscription in the context of streams and futures. I've left comments on the diff with specific questions. If this first part is settled, I'll push another commit with the actual stream implementation. |
arg0: Option<&str>, | ||
flags: DBusSignalFlags, | ||
callback: P, | ||
) -> SignalSubscriptionId { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdroege This is incompatible, obviously. Is this okay? Or would you rather have me mark signal_subscribe
and signal_unsubscribe
as deprecated, and add a new separate method. If so, can you suggest a new for this new method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking the API is OK but we obviously can't backport this then and it will have to wait until the next breaking release next summer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do it in two steps: one that deprecate the current methods & add new ones & a new commit dropping the deprecated methods & rename the new ones. This way we can backport only the first commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two steps, but in MR? That I can do. Suggestions for a new name? I'd go for subscribe_signal
, which is arguably the nicer name anyway. Slightly confusing if we have signal_subscribe
and subscribe_signal
but if the former is clearly deprecated it's perhaps fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this as a separate subscribe_to_signal
method, and marked signal_subscribe
and signal_unsubscribe
as deprecated. I'm open for a better name for the new method.
I didn't remove signal_subscribe
and signal_unsubscribe
, because the new method is actually implemented on top of these, which sort of nicely separates the FFI binding boilerplate from the Rust-level interface, and signal_subscribe
at least also offers valuable documentation which I couldn't copy to the new method due to the licensing situation.
In the next breaking release you could then either just remove the pub
, or completely inline both, or even indefinitely retain them as deprecated methods, if only for documentation.
struct SignalSubscriptionId(NonZeroU32); | ||
|
||
#[derive(Debug)] | ||
pub struct SignalSubscription(DBusConnection, Option<SignalSubscriptionId>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdroege In this implementation, the signal subscription keeps a strong reference to the connection so a signal subscription in and by itself is sufficient to keep the connection alive, in my understanding.
I think that's what you'd normally want to have, but I also thought about making the connection a weakref here. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it keeps a strong reference then you'd easily create reference cycles, or not? A weak reference might be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly.
But on the other hand, if the subscription just holds a weak reference then holding on to the subscription alone wouldn't actually maintain the subscription, would it? I'm not too familiar with GDBus, but presumably all signal subscriptions stop when the connection is destroyed.
So if the subscription doesn't hold a strong reference to the connection, then something like the following:
let connection = gio::bus_get_future(BusType::Session).await?;
let subscription = connection.signal_subscribe(...)?;
self.subscription_to_my_important_signal.replace(Some(subscription))
wouldn't actually work, would it?
The new connection
drops when leaving the surrounding block, there's no other strong reference to it, so the connection would be destroyed, taking the signal with it, and the callback given to .signal_subscribe
would never be invoked.
I believe that this would be very surprising, and debugging things that don't happen is a bloody nuisance, so this API would arguably not follow the principle of least surprise.
How about adding a .downgrade()
method on signal subscription to convert to a WeakSignalSubscription
which only holds a weak reference? That'd follow the normal API pattern here (APIs normally return strong references and need to be downgraded explicitly), and downstream consumers could decide whether the need a weak or a strong ref.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a WeakSignalSubscription
counterpart with converse upgrade
/downgrade
methods, so that the call to subscribe_to_signal
now behaves as I believe is least surprising, while still allowing the caller to break reference cycles by downgrading to a weak reference on connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe some example for this would be useful to have. I don't know how this would be usually used :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh, I had hoped that it's sort of obvious 😅
Essentially you just keep the returned subscription somewhere and drop it when you no longer want to receive the signal. The default subscription keeps a strong reference on the connection, the weak one keeps a weak reference.
I had hoped that this simplifies lifecycle management for signal subscriptions, but if it actually needs an example to be understandable, the API is probably not that good?😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that part is clear. I mean, how would application code making use of all this actually look like? Keep in mind that I only have knowledge about how DBus works, I never really wrote code for it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. I'll make an example then which shows how to use this API in different ways 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdroege I added a simple example; is that what you had in mind?
PS: I'm not really following the CI errors; are these mine? |
No, you can ignore them. Things changed on the github side and now stuff is failing. |
267196d
to
43b4e8b
Compare
@sdroege @bilelmoussaoui I've addressed your answers to my questions, and while I was at it I also pushed a second commit which replaces the lengthy callback signature with four anonymous I don't cling to this tho, so if you dislike that I can back out that commit. If you agree with the proposed API I'd continue with implementing the |
43b4e8b
to
38ab162
Compare
Add a new method subscribe_to_signal which returns a managed signal subscription which unsubscribes from the signal when dropped. Also add a weak ref counterpart to this signal subscription to allow breaking reference cycles. This fits better into Rust's ownership model and makes it much easier to handle the lifetime of a signal subscription in async code. Mark the old signal_subscribe and signal_unsubscribe as deprecated in favour of this new subscribe_to_signal method.
The signal_subscribe callback signature is hard to understand and easy to get subtly wrong, with its four unnamed str arguments. In subscribe_to_signal introduce a new DBusSignalRef struct and put all received parameters into this struct, to provide callback users with "named" arguments which are easier to understand and play well with e.g. editor auto-completion.
38ab162
to
0913fcb
Compare
0913fcb
to
9282073
Compare
@sdroege @bilelmoussaoui I went ahead and added |
c319d10
to
c31cb16
Compare
c31cb16
to
69c32a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me otherwise
69c32a3
to
f635475
Compare
As briefly discussed in #1820 I'd like to add means to receive signals as streams, which is a lot more convenient to use than callbacks.
Closes #1820