Skip to content

Implement Send on Cursors so we can move them between threads #334

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Kerollmops
Copy link
Member

This PR implements Send on the LMDB cursors struct wrapper. The goal is to move database iterators between threads.
If this PR is merged, we must release a v0.22.1 patch. This change is not breaking.


Hey @hyc 👋

Is it valid to move cursors between threads? Nothing in the documentation seems to prevent that, and nothing in the code prevents it either. Note that cursors are always deallocated when dropped (going out of scope). So, I only fear that we create a cursor on one thread and deallocate it on another. However, allocations are done with malloc, so it looks safe.

Thank you and have a lovely week 🍀

@xav-db
Copy link

xav-db commented May 29, 2025

can we get this merged soon as I really want to use this!

@Kerollmops
Copy link
Member Author

Hey @xav-db 👋

I cannot merge this as-is, as the behavior could break the wrapper and some invariants. However, I suggest you test that independently and tell me how your program is doing. Ultimately, I'd like a review from someone more knowledgeable than me on C stuff and thread safety.

Take care 🌵

Copy link
Contributor

@antonilol antonilol left a comment

Choose a reason for hiding this comment

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

I think implementing Send should be fine, the docs start with saying "The library is fully thread-aware".

Also, if desired, Sync can be implemented on any type without methods that take &self, for free. See std::sync::Exclusive for more info.

@Kerollmops
Copy link
Member Author

Hey @antonilol 👋

I think implementing Send should be fine, the docs start with saying "The library is fully thread-aware".

Unfortunately, you cannot trust the sole documentation of a C library. I don't know what "The library is fully thread-aware" exactly means but here is an example of non-thread awareness. Another is that it is not valid to implement Sync on a Cursor (internal) and use this cursor on multiple threads without synchronization.

Also, if desired, Sync can be implemented on any type without methods that take &self, for free. See std::sync::Exclusive for more info.

Very interesting, thanks. However, this is still nightly. We cannot rely on this in heed. At least, for now.

@antonilol
Copy link
Contributor

Another is that it is not valid to implement Sync on a Cursor (internal) and use this cursor on multiple threads without synchronization.

Isn't this impossible already because to use a cursor, &mut self is needed, which can only exist at one place at a time. This is also the reason why Sync can be implemented on any type that does not take self by reference (&self). ("Every method that takes self by reference does proper synchronization" is vacuously true then)

Very interesting, thanks. However, this is still nightly. We cannot rely on this in heed. At least, for now.

I meant this as an example for how Sync can be implemented for free, Sync works like this since the first release of stable Rust, only this wrapper is unstable.

@antonilol
Copy link
Contributor

Unfortunately, you cannot trust the sole documentation of a C library. I don't know what "The library is fully thread-aware" exactly means but here is an example of non-thread awareness.

This is true, we will have to wait until the author of LMDB responds or someone checks the C code cursors use.

@xav-db
Copy link

xav-db commented Jul 17, 2025

@lukasnxyz

@xav-db
Copy link

xav-db commented Jul 17, 2025

@RaphaelDarley

@xav-db
Copy link

xav-db commented Jul 17, 2025

going to go through the C code

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