Skip to content
This repository has been archived by the owner on May 11, 2023. It is now read-only.

Allow setting timeout for blocking remote calls #113

Open
zeenix opened this issue Nov 4, 2020 · 7 comments
Open

Allow setting timeout for blocking remote calls #113

zeenix opened this issue Nov 4, 2020 · 7 comments
Labels
enhancement New feature or request trivial zbus Issues/PRs related to zbus crate

Comments

@zeenix
Copy link
Collaborator

zeenix commented Nov 4, 2020

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
Copy link
Collaborator Author

zeenix commented Nov 18, 2020

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
Copy link
Collaborator Author

zeenix commented Nov 18, 2020

@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
Copy link
Collaborator Author

zeenix commented Nov 18, 2020

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
Copy link
Collaborator Author

zeenix commented Nov 19, 2020

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
Copy link
Collaborator Author

zeenix commented Nov 19, 2020

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
Copy link
Collaborator Author

zeenix commented Nov 19, 2020

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
Copy link
Collaborator Author

zeenix commented Mar 31, 2022

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

This came up in dark-light: frewsxcv/rust-dark-light#17

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request trivial zbus Issues/PRs related to zbus crate
Projects
None yet
Development

No branches or pull requests

1 participant