-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this shouldn't be a reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! I like the direction and have some preliminary comments and questions.
rustler/src/types/local_pid.rs
Outdated
@@ -8,6 +8,9 @@ pub struct LocalPid { | |||
} | |||
|
|||
impl LocalPid { | |||
pub unsafe fn new(c: ErlNifPid) -> LocalPid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe from_raw
?
rustler/src/resource.rs
Outdated
) { | ||
unsafe { | ||
let pid = LocalPid::new((&*pid).clone()); | ||
let mon = Monitor { inner: (&*mon).clone() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Monitor::from_raw(..)
?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is pub(crate)
enough?
rustler/src/resource.rs
Outdated
} | ||
} | ||
|
||
fn maybe_env(caller_env: Option<&Env>) -> NIF_ENV { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) :) |
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
,Monitor
andMonitorResource
.