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

Ensure that native C++ index object doesn't get destroyed while Java threads are using it. #36

Merged
merged 19 commits into from
Mar 19, 2024

Conversation

psobot
Copy link
Member

@psobot psobot commented Oct 26, 2023

This PR adds a somewhat clever solution to give us a bit more thread safety around our JNI bindings to Voyager's C++ code: instead of storing a raw pointer on the Java object, we now store a pointer to a std::shared_ptr, which gives us thread-safe destruction and ensures that the underlying C++ object doesn't get destroyed while another Java thread might still be using it.

@psobot psobot added the bug Something isn't working label Oct 26, 2023
@psobot psobot requested a review from dylanrb123 October 26, 2023 13:31
@samek
Copy link
Contributor

samek commented Oct 26, 2023

@psobot So I should remove #35 since it calls index the old way, and put in another one once this one is merged?

@psobot
Copy link
Member Author

psobot commented Oct 26, 2023

No need, @samek - if your PR gets merged first, I'll update this one (as it'll fail to compile), and vice versa.

@psobot psobot requested review from markkohdev and dylanrb123 and removed request for dylanrb123 November 14, 2023 02:22
@markkohdev
Copy link
Contributor

@psobot is there a good way to test this to ensure that it's working as expected?

@psobot
Copy link
Member Author

psobot commented Dec 6, 2023

@markkohdev test added!

@psobot psobot merged commit 6ffbf68 into main Mar 19, 2024
53 checks passed
@psobot psobot deleted the psobot/destructor-lock branch March 19, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants