Instance-specific send and recieve functions#24
Open
minerscale wants to merge 2 commits intoalisomay:mainfrom
Open
Instance-specific send and recieve functions#24minerscale wants to merge 2 commits intoalisomay:mainfrom
minerscale wants to merge 2 commits intoalisomay:mainfrom
Conversation
Owner
|
Sorry that I missed this one. |
Author
|
Hi @alisomay, Any updates on getting this merged, also relatedly getting alisomay/libpd-sys#7 merged? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As of current if you try to call almost any function in
libpd_rs::functionsfrom 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 thePdstruct to include all the functions inlibpd_rs::functions::sendandlibpd_rs::functions::recieve.By calling these from the
Pdstruct it becomes impossible to use these functions from a separate thread sincePdis notSendorSync. I also placeActiveInstanceGuards 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_toandfinish_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
Callbackswhich 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 thePdstruct 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::functionsone 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 inlibpd_rs::functionsbe 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_messagetest which I can't do this because otherwise adspmessage is sent to theon_messagetest. 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 calledGUARD_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!