Skip to content

Instance-specific send and recieve functions#24

Open
minerscale wants to merge 2 commits intoalisomay:mainfrom
minerscale:main
Open

Instance-specific send and recieve functions#24
minerscale wants to merge 2 commits intoalisomay:mainfrom
minerscale:main

Conversation

@minerscale
Copy link

@minerscale minerscale commented Aug 8, 2025

As of current if you try to call almost any function in libpd_rs::functions from a different thread than the context was created, it will segfault, this of course makes perfect sense given that each context is thread-specific but it is my opinion that behavior should not be able to happen in the first place. This pull request extends the Pd struct to include all the functions in libpd_rs::functions::send and libpd_rs::functions::recieve.

By calling these from the Pd struct it becomes impossible to use these functions from a separate thread since Pd is not Send or Sync. I also place ActiveInstanceGuards on each invocation of the functions to ensure that the correct instance is used, although I haven't tested whether or not having multiple instances running on the same thread works, I assume it's not worse than it was when I started.

Additionally, runtime checks for the invariants of buffer overflows and not adding to messages that haven't yet been started yet have been added for start_message, add_x_to_started_message, finish_message_as_list_and_send_to and finish_message_as_typed_message_and_send_to.

I also ensure that when the Pd instance is dropped, the supplied callbacks for event listeners are properly cleaned up instead of leaking the memory. I achieve this with a somewhat-hacky but performant internal type called Callbacks which manages a Vec of destructors to call when Pd drops. Perhaps it would be better to drop the callbacks when they are replaced rather than dropping them at the end of the lifetime of the Pd instance but it's still much better than it was before, where callbacks simply leaked memory for the duration of the running program. In the end my changes add a few bytes to the size of the Pd struct for the cost of dramatically improved safety.

This PR updates all the tests to use the new Pd struct API rather than old libpd_rs::functions one because I believe that this really should be the correct way to use it. As of now I have implemented these changes to be in what I believe to be mostly non-breaking. However I believe if you were to go make breaking changes I think that almost every function if not every function in libpd_rs::functions be marked as unsafe, since they are able to segfault.

The last thing I did was add a check in the callback registration about registering callbacks while the DSP is active. The docs say not to but the tests all do this. I've rearranged most of the tests around to avoid this except the send_and_receive_typed_message test which I can't do this because otherwise a dsp message is sent to the on_message test. I could fix this, but I figured that I should probably just ask what the documentation note "Note: Do not register this listener while pd DSP is running." is actually for, since the examples seem to work anyway. The check is currently disabled behind a flag called GUARD_FROM_CALLBACK_DURING_DSP. Enable it if you would like or remove the check if you think it is unnecessary.

This change might be a bit opinionated but when loading libpd it spews a bunch of version info out to stderr which is really unbecoming of a library and very annoying, I couldn't work out how to shut it up using calls to the C API so I've shut it up with the nifty gag crate. This makes the tests way less noisy and hopefully results in a better experience for everyone using the library, although it does suppress potentially useful debug information, so I certainly understand wanting to revert this change.

These changes are clippy passing and the docs compile nicely without errors too. I hope these changes are useful to you!

@alisomay
Copy link
Owner

alisomay commented Sep 6, 2025

Sorry that I missed this one.
I'll review as soon as I have time but just skimmed through and it looks good generally.
Thanks for the effort ✨

@minerscale
Copy link
Author

Hi @alisomay,

Any updates on getting this merged, also relatedly getting alisomay/libpd-sys#7 merged?

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.

2 participants