rusterlium / rustler

Safe Rust bridge for creating Erlang NIF functions
https://docs.rs/crate/rustler
Apache License 2.0
4.32k stars 225 forks source link

feature: compare local pids #611

Closed hengermax closed 4 months ago

hengermax commented 4 months ago

For a feature in a library we're writing we wanted to compare LocalPids without encoding/decoding them into Terms. For this I thought the process was simple:

However, when I'm running all the test suites (specifically the rustler_tests one by calling mix test) I'm now running into:

{:error,
 {:load_failed,
  ~c"Failed to load NIF library: 'dlopen(/Users/maxh/Development/rustler/rustler_tests/_build/test/lib/rustler_test/priv/native/librustler_test.so, 0x0002): symbol not found in flat namespace '_enif_compare_pids''"}}

I know this implies it cannot find the symbol in the erlang libs, but I was unable to find the location where the libraries are included to figure out which specific symbol I should be including.

Hence two questions:

  1. Would you want to include this feature for LocalPid?
  2. If so: could somebody help to point me in the right direction to make sure I'm referencing the right symbol?

Additional information:

evnu commented 4 months ago

I believe enif_compare_pids is not a function, but a macro. Thus, there isn't a symbol for it.

hengermax commented 4 months ago

Welp, that is a bit silly from me. Before I "found" that enif_compare_pids I hopped through a lot of C macros to figure out what enif_compare was doing under the hood. But at no point did I consider the possibility that my missing symbol would be missing because it was actually a macro... Would you still accept the PR if I would implement the comparisons?

I have two options. Note that Term::cmp calls enif_compare using Term::as_c_arg as the arguments. That c_arg, looking at LocalPid::encoder, is equal to pid::make_pid(env.as_c_arg(), self.c). Which is an alias for enif_make_pid, which is actually manually implemented to just return the ErlNifPid::pid field, without referring to the Env.

So, if you would allow this, we could:

  1. drop the env argument to pid::make_pid and enif_make_pid. So we may call enif_compare directly (but it is a bit ugly, as we're no longer following the interface as specified in the erlang documentation).
  2. Hardcode a enif_compare_pids, just like enif_make_pid, which would simply call enif_compare with the internal ErlNifPid::pid field. Just like the C macro does in the link you provided.
  3. ???

I believe (2) is a reasonable solution. What do you think?

filmor commented 4 months ago

Yes, (2) would be an acceptable solution. I'd rather not have us change interfaces of wrapped functions, there is no guarantee that they won't actually require their parameters at some point.

filmor commented 4 months ago

Looks good to me. We'll have to follow this up with a minor version bump on rustler-sys and a corresponding bump of the dependency in rustler.

hengermax commented 4 months ago

Thanks for the quick replies and the nice library!

hengermax commented 4 months ago

Just to be sure: do you want me to bump the version or will somebody else?

filmor commented 4 months ago

It's a note for myself, I will do that after this one is merged :). Just waiting for another review from @evnu before merging.