RPC Rust Runtime #183
RPC Rust Runtime #183
Conversation
# Conflicts: # Compiler/Properties/launchSettings.json
# Conflicts: # Laboratory/Rust/benchmarking/src/bebops/jazz.rs
|
I think this needs to be rebased into a feature branch. I know we discussed it briefly but i’d like to avoid massive changes like this going into master until it’s ready across the board. |
| /// Define a timeout value for RPC. | ||
| /// This creates an Option<Duration> value which is what RPC requests expect. | ||
| /// | ||
| /// Use as `timeout!(n u)` where `n` is a numeric and `u` is the unit. Example `timeout!(4 s)` | ||
| /// creates a 4 second timeout. `timeout!(None)` defines no timeout. | ||
| #[macro_export] | ||
| macro_rules! timeout { |
There was a problem hiding this comment.
I think we don't need this macro, and making people write Some(Duration::from_secs(4)) is okay and probably more idiomatic.
There was a problem hiding this comment.
The reason I think we should keep it is because writing
let resp = router.remote_call(Some(Duration::from_secs(4)), arg1, arg2).await;is both more verbose making it less likely someone will bother to follow best practice, and also is less easy to read than
let resp = router.remote_call(timeout!(4 s), arg1, arg2).await;| pub fn convert_timeout(timeout: Option<Duration>) -> Option<NonZeroU16> { | ||
| timeout.and_then(|t| { | ||
| let t = t.as_secs(); | ||
| debug_assert!(t < u16::MAX as u64, "Maximum timeout is 2^16-1 seconds."); |
There was a problem hiding this comment.
Should this be a user-facing error rather than just an assertion? A user might not know about this restriction and define an RPC with a large timeout, right?
There was a problem hiding this comment.
Maybe? I was thinking it would usually be something they write into the code so it would be a code bug.
| /// **Warning:** always store weak references or the router will have issues shutting down due | ||
| /// to cyclical dependencies. | ||
| #[doc(hidden)] | ||
| pub _context: Arc<RouterContext>, |
There was a problem hiding this comment.
Can we not change the type to Weak<RouterContext> ?
There was a problem hiding this comment.
No, this is the only owner.
|
Siding with @andrewmd5 I think making an |
No description provided.