zbus-old icon indicating copy to clipboard operation
zbus-old copied to clipboard

Allow setting timeout for blocking remote calls

Open zeenix opened this issue 5 years ago • 7 comments
trafficstars

This will be mainly

  • Connection::
    • send_message
    • call_method
    • receive_message

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 avatar Nov 04 '20 14:11 zeenix

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 avatar Nov 18 '20 20:11 zeenix

@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 avatar Nov 18 '20 21:11 zeenix

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 avatar Nov 18 '20 21:11 zeenix

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 avatar Nov 19 '20 00:11 zeenix

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 avatar Nov 19 '20 00:11 zeenix

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 avatar Nov 19 '20 00:11 zeenix

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

zeenix avatar Mar 31 '22 01:03 zeenix