[wip] add support for monitors #376
Conversation
rustler/src/resource.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn maybe_env(caller_env: Option<&Env>) -> NIF_ENV { |
There was a problem hiding this comment.
I'm not sure if this is necessary. The docs state Argument caller_env is the environment of the calling thread (process bound or callback environment) or NULL if calling from a custom thread not spawned by ERTS.. However looking at the source code, env isn't even used. But in older versions of OTP (and maybe in the future again?) it was used, so it might be safer to keep this logic (which is currently just a copy-paste from OwnedEnv)
There was a problem hiding this comment.
If the documentation says so, I think we should stick with the documented behaviour. But maybe we could turn this into a helper function instead of duplicating code? E.g. Env::called_from_thread(env)?
rustler/src/resource.rs
Outdated
| inner: align_alloced_mem_for_struct::<T>(handle) as *mut T, | ||
| raw: handle | ||
| }; | ||
| T::down(&resource, pid, mon); |
There was a problem hiding this comment.
Probably this shouldn't be a reference
rustler/src/types/local_pid.rs
Outdated
| @@ -8,6 +8,9 @@ pub struct LocalPid { | |||
| } | |||
|
|
|||
| impl LocalPid { | |||
| pub unsafe fn new(c: ErlNifPid) -> LocalPid { | |||
rustler/src/resource.rs
Outdated
| ) { | ||
| unsafe { | ||
| let pid = LocalPid::new((&*pid).clone()); | ||
| let mon = Monitor { inner: (&*mon).clone() }; |
| let pid = LocalPid::new((&*pid).clone()); | ||
| let mon = Monitor { inner: (&*mon).clone() }; | ||
| crate::wrapper::resource::keep_resource(handle); | ||
| let resource = ResourceArc { |
There was a problem hiding this comment.
I would expect this to be a helper function in impl<T> ResourceArc<T>.
| down: *const ErlNifResourceDown, // enif_monitor_process | ||
| members: c_int, | ||
| dyncall: *const ErlNifResourceDynCall, | ||
| pub dtor: Option<ErlNifResourceDtor>, |
rustler/src/resource.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn maybe_env(caller_env: Option<&Env>) -> NIF_ENV { |
There was a problem hiding this comment.
If the documentation says so, I think we should stick with the documented behaviour. But maybe we could turn this into a helper function instead of duplicating code? E.g. Env::called_from_thread(env)?
|
Thanks for the review! I'll try to implement your suggestions soon(ish) :) |
dlesl
force-pushed
the
monitors
branch
5 times, most recently
from
c693319
to
d68f109
Compare
|
Thank you for your contribution. I took most of your tests, but had to implement things a bit differently to make it work with the refactored resource implementation. |
This is still at a PoC stage but I'm opening this PR to get some feedback :). There's at least one thing that I think definitely needs some work, the naming of
ResourceArcMonitor,MonitorandMonitorResource.