Description
As @kjvalencik points out in neon-bindings/neon#551 and neon-bindings/neon#552, the memory management model for EventHandler is still not safe: it's susceptible to leaks and races.
The problem is that the we aren't coordinating the overall lifetime of the queue of pending invocations of the event handler (which all run on the main thread); we're only tracking the places in random other threads where invocations are requested. So when the last request is made, we drop the underlying queue even though there may be pending invocations.
Instead, I propose we expose the ownership model into the public API of EventHandler. That is, the user should understand EventHandler as an atomically refcounted handle to a JS event handler callback. The relevant changes to the API would be:
- Scheduling an invocation of the event handler consumes
self. - In order to schedule multiple invocations of the event handler, you have to explicitly
.clone()it.
The types would look something like this:
struct InvocationQueue(...); // private implementation
impl Drop for InvocationQueue { ... } // private implementation
#[derive(Clone)]
pub struct EventHandler(Arc<InvocationQueue>);
impl EventHandler {
pub fn new<'a, C: Context<'a>, T: Value>(cx: &C, this: Handle<T>, callback: Handle<JsFunction>) -> Self;
// CHANGE: consumes self (implementation clones the Arc<InvocationQueue> and sends to main thread)
pub fn schedule<T, F>(self, arg_cb: F)
where T: Value,
F: for<'a> FnOnce(&mut EventContext<'a>) -> JsResult<'a, T>,
F: Send + 'static;
// CHANGE: consumes self (implementation clones the Arc<InvocationQueue> and sends to main thread)
pub fn schedule_with<F>(self, arg_cb: F)
where F: FnOnce(&mut EventContext, Handle<JsValue>, Handle<JsFunction>) -> NeonResult<()>,
F: Send + 'static;
}Notice that the implementation of the schedule* methods would need to clone the Arc<InvocationQueue> and send it to the main thread in order to ensure that it's kept alive until all the invocations have terminated. This prevents the race where the invocation queue is shut down before all the invocations have executed.
This should also make EventHandler virtually leak-proof: the only way to keep it alive indefinitely is to either keep a local computation running infinitely or to keep cloning it and passing it on to other computations infinitely. Otherwise, by default it will be shut down once all cloned instances have either dropped or scheduled and executed their invocations.
This is an example of what would look different in user code. The example from the RFC would just need one more line to explicitly clone the EventHandler on each iteration:
let mut this = cx.this();
let cb = cx.argument::<JsFunction>(0)?;
let handler = EventHandler::new(cb);
// or: = EventHandler::bind(this, cb);
thread::spawn(move || {
for i in 0..100 {
// do some work ....
thread::sleep(Duration::from_millis(40));
// schedule a call into javascript
let handler = handler.clone(); // CHANGE: clone the handler before scheduling an invocation
handler.schedule(move |cx| {
// successful result to be passed to the event handler
Ok(cx.number(i))
}
}
});