Closed zeenix closed 1 year ago
@de-vri-es Yes we're aware that the lifetime isn't needed but the problem is that removing that lifetime would be a breaking change.
The lifetime now prevents moving the stream into a spawned task. As a result, all handling of signals must now happen in the same task where the connection was made. This is quite a big restriction
That would indeed be a big restriction if that was true. Firstly a connection is cheaply clonable and implements Send
so you can create proxies from the same underlying connection in multiple tasks/threads.
Secondly, the lifetime of SignalStream
is not bound to the Proxy
itself but the inner lifetime of Proxy
. i-e SignalStream
can outlive the Proxy
it was created from. The bound is on the strings (path, interface and destination). If you use strings with 'static
lifetime, then your SignalStream
also gets a 'static
lifetime. Note that SignalStream
implements Send + Sync + Unpin
.
In GitLab by @de-vri-es on Feb 13, 2023, 13:06
That would indeed be a big restriction if that was true. Firstly a connection is cheaply clonable and implements
Send
so you can create proxies from the same underlying connection in multiple tasks/threads.
That's nice, but that means that if you want to handle signals in a spawned task you have to:
proxy.connection()
).Result
).Instead of:
Secondly, the lifetime of
SignalStream
is not bound to theProxy
itself but the inner lifetime ofProxy
. i-eSignalStream
can outlive theProxy
it was created from. The bound is on the strings (path, interface and destination). If you use strings with'static
lifetime, then yourSignalStream
also gets a'static
lifetime. Note thatSignalStream
implementsSend + Sync + Unpin
.
It is using the inner lifetime of the
I see this assumption was wrong, the lifetime of the Proxy
, but that lifetime is the lifetime of the Connection
object, is it not? It doesn't seem related to the lifetime of string names. The returned SignalStream
has lifetime 'a
, not 'm
:Connection
is not the same as 'a
.
Since the lifetime is not actually used, it should be relatively easy to add a new function that returns the same object with a 'static
lifetime. This is not ideal from a clean API perspective, but it does significantly simplify handling signals in a spawned task.
In GitLab by @de-vri-es on Feb 13, 2023, 13:09
Or, somewhat odd, SignalStream
could gain a member function to consume self
and return a new SignalStream<'static>
.
This has the advantage that it only requires one new member function on SignalStream
rather than duplicating all functions that return a SignalStream
.
In GitLab by @de-vri-es on Feb 13, 2023, 13:16
I see my assumptions were incorrect. I thought that the lifetime 'a
of Proxy<'a>
was the same as the lifetime of &Connection
. However, that is a distinct lifetime, so I think I can probably change all my Proxy
lifetimes to 'static
.
Thank you for your answer, and apologies for the noise!
right. :) Np.
What we can do w/o breaking API is mark receive_signal
methods return SignalStream<'static>
so it's clear to people.
In GitLab by @de-vri-es on Feb 13, 2023, 11:26
The
SignalStream<'a>
has a lifetime that isn't actually used:I imagine this was added to prevent the stream from outliving the proxy object. However, a
Stream
already has an interface for dealing with that situation: it can returnReady(None)
frompoll_next()
when it will no longer produce any value.The lifetime now prevents moving the stream into a spawned task. As a result, all handling of signals must now happen in the same task where the connection was made. This is quite a big restriction, and it seems artificial in the case of a
SignalStream
.