register_blocking_method
#523
Conversation
Somewhere here I think would be a good place, possibly also with a machine checked example. |
| @@ -466,6 +466,43 @@ impl<Context: Send + Sync + 'static> RpcModule<Context> { | |||
| Ok(MethodResourcesBuilder { build: ResourceVec::new(), callback }) | |||
| } | |||
|
|
|||
| /// Register a new **blocking** synchronous RPC method, which computes the response with the given callback. | |||
| /// Unlike the regular [`register_method`](RpcModule::register_method), this method can block its thread and perform expensive computations. | |||
| pub fn register_blocking_method<R, F>( | |||
There was a problem hiding this comment.
Soon we'll be at a point where we should have a macro for all these register_* methods... :)
| } | ||
|
|
||
| // Each request takes 50ms, added 10ms margin for scheduling | ||
| assert!(elapsed < Duration::from_millis(60), "Expected less than 60ms, got {:?}", elapsed); |
There was a problem hiding this comment.
I'm hard pressed to come up with something better than this. It'll have to make do.
There was a problem hiding this comment.
Right now on MacOS CI 50ms + 50ms in parallel = 160ms though :D
| let ctx = self.ctx.clone(); | ||
| let callback = self.methods.verify_and_insert( | ||
| method_name, | ||
| MethodCallback::new_async(Arc::new(move |id, params, tx, claimed| { |
There was a problem hiding this comment.
dq: Why can't we do new_sync i.e, the closure itself is synchronous?
hrm, I assume using tokio::spawn_blocking is more efficient than std::thread::spawn because tokio is already initialized when the server is started?
Should we have a way to register an async closure too or is that the footgun you were talking about?
There was a problem hiding this comment.
dq: Why can't we do
new_synci.e, the closure itself it synchronous?hrm, I assume using
tokio::spawn_blockingis more efficient thanstd::thread::spawnbecause tokio is already initialized when the server is started?
Yeah, tokio will manage a threadpool for blocking tasks, spawning OS threads can be costly (pending on how blocking the callback really is). That said, we can probably go with new_sync here anyway, I thought we need to track the future to completion, but we don't really...
Should we have a way to register an async closure too or is that the footgun you were talking about?
Yeah, that's the footgun. Would have to wrap the async callback into something like LocalSet that's bound to the thread, but the moment you do some .await on something that might be expecting a regular tokio runtime (for io or timers or something) I'm not sure exactly what would happen, but I wouldn't be surprised to see a panic.
There was a problem hiding this comment.
Tried to make it work with sync callback, but there are some assumptions about lifetimes for params sync callback makes, plus it also means I need to pass in claimed resources handler, so all in all using async callback is way less invasive for what is ultimately a niche feature.
Potentially fixes #486.
Not sure where a good place to document the
blockingflag in the#[method]macro attribute would be?