Skip to content
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

feature: compare local pids #611

Merged
merged 5 commits into from
May 29, 2024
Merged

Conversation

hengermax
Copy link
Contributor

@hengermax hengermax commented May 27, 2024

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:

  • Add nif_compare_pids to rustler_sys (by adding a b.func under the appropriate version). See: enif_compare_pids
  • Implement PartialEq/Eq/PartialOrd/Ord for LocalPid
  • Write some tests

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:

  • I'm running OSX
  • If I switch back to the master branch (and clear all /target, /_build folders) I can run the tests without issues.

@evnu
Copy link
Member

evnu commented May 27, 2024

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

@hengermax
Copy link
Contributor Author

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
Copy link
Member

filmor commented May 27, 2024

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.

@hengermax hengermax marked this pull request as ready for review May 28, 2024 07:46
@filmor
Copy link
Member

filmor commented May 28, 2024

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
Copy link
Contributor Author

Thanks for the quick replies and the nice library!

@hengermax
Copy link
Contributor Author

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

@filmor filmor requested a review from evnu May 28, 2024 12:24
@filmor
Copy link
Member

filmor commented May 28, 2024

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

Copy link
Member

@evnu evnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@filmor filmor merged commit 127e255 into rusterlium:master May 29, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants