dbus2 / zbus-old

Rust D-Bus crate.
https://gitlab.freedesktop.org/dbus/zbus
Other
49 stars 13 forks source link

Allow setting timeout for blocking remote calls #113

Open zeenix opened 3 years ago

zeenix commented 3 years ago

This will be mainly

Probably best we have a timeout on the whole Connection itself and add a getter/setter for timeout instead of adding _timeout variants of these methods.

Notes for implementor

There are setters for timeouts in std that you can make use of. Since std crate separates read and write timeouts, it'd make sense for us to do the same I think.

Update: now that our primary API is async and both tokio and async-io provide an easy way to add timeouts to async calls, this becomes much easier.

zeenix commented 3 years ago

In GitLab by @codido on Nov 18, 2020, 21:53

@zeenix This might be a bit more complicated than just setting the socket timeouts. A timed out operation could result in EAGAIN/EWOULDBLOCK which might break some assumptions in the code, ending up blocking in poll() indefinitely.

zeenix commented 3 years ago

@zeenix This might be a bit more complicated than just setting the socket timeouts. A timed out operation could result in EAGAIN/EWOULDBLOCK which might break some assumptions in the code, ending up blocking in poll() indefinitely.

Thanks for pointing out. I didn't think of that. I think we should be OK though as long as our own code takes care of that (i-e if time-out is set, return the appropriate error to user instead of trying again implicitly) and we document this behavior. Correct?

zeenix commented 3 years ago

In GitLab by @codido on Nov 18, 2020, 22:45

This is certainly a valid approach, but one thing to keep in mind is that the current API lets users set or get the socket directly. So figuring out if a socket is blocking or non-blocking based on the potential timeout calls might break if someone does something like:

let fd = connection.as_raw_fd();
SendTimeout.set(fd, &TimeVal::milliseconds(100))?;

..or even just sets the timeout before passing the socket to ClientHandshake to begin with.

zeenix commented 3 years ago

but one thing to keep in mind is that the current API lets users set or get the socket directly. So figuring out if a socket is blocking or non-blocking based on the potential timeout calls might break

Right but we don't have to rely on the timeout event but can check it directly from the socket itself.

zeenix commented 3 years ago

but one thing to keep in mind is that the current API lets users set or get the socket directly. So figuring out if a socket is blocking or non-blocking based on the potential timeout calls might break

Right but we don't have to rely on the timeout event but can check it directly from the socket itself.

Oops, you were talking about blocking vs. non-blocking. Why would a timeout result in EWOULDBLOCK error btw? Even if that's the case, I wouldn't be too concerned about this kind of corner case, as long as we document it.

zeenix commented 3 years ago

In GitLab by @codido on Nov 19, 2020, 01:27

That's the documented behaviour it seems, as documented for SO_RCVTIMEO and SO_SNDTIMEO: https://linux.die.net/man/7/socket

zeenix commented 2 years ago

In GitLab by @Be on Mar 31, 2022, 03:55

This came up in dark-light: https://github.com/frewsxcv/rust-dark-light/issues/17